Bug 1368030 - Fix race condition in ScreenCapturerMac. r=jesup
☠☠ backed out by ac69b48a3503 ☠ ☠
authorDan Minor <dminor@mozilla.com>
Wed, 19 Jul 2017 14:49:05 -0400
changeset 419482 0a60cc1983211905a5bc0aaf9e3414469add6b91
parent 419481 22649352b9f4137bb410fcfffbd1c2fa5ca0659c
child 419483 839d8a60a2eaf716fe4dbc2296cfd6bc6703e3db
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup
bugs1368030
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368030 - Fix race condition in ScreenCapturerMac. r=jesup The race condition is between ~ScreenCapturerMac and the ScreenRefresh and ScreenUpdateMove callbacks. The destructor calls UnregisterRefreshAndMoveHandlers but a callback may still occur after the destruction of the object. Rather than passing a pointer to ScreenCapturerMac into the callback, this adds a separate object which keeps a pointer to ScreenCapturerMac guarded by a CriticalSection. The destructor sets the ScreenCapturerMac to nullptr. In the next callback, the handler unregisters the callbacks and deletes the object. The downside to this approach is that if the ScreenCapturerMac object is allocated and deallocated before a callback occurs, the memory for the separate object will be leaked.
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm
--- a/media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm
+++ b/media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm
@@ -24,16 +24,17 @@
 #include <dlfcn.h>
 #include <IOKit/pwr_mgt/IOPMLib.h>
 #include <OpenGL/CGLMacro.h>
 #include <OpenGL/OpenGL.h>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/macutils.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/modules/desktop_capture/desktop_capturer.h"
 #include "webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
 #include "webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "webrtc/modules/desktop_capture/desktop_region.h"
 #include "webrtc/modules/desktop_capture/mac/desktop_configuration.h"
 #include "webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h"
@@ -326,16 +327,24 @@ class ScreenCapturerMac : public Desktop
                         const CGRect *rect_array);
   static void ScreenRefreshCallback(CGRectCount count,
                                     const CGRect *rect_array,
                                     void *user_parameter);
   static void ScreenUpdateMoveCallback(CGScreenUpdateMoveDelta delta,
                                        size_t count,
                                        const CGRect *rect_array,
                                        void *user_parameter);
+  struct ScreenCallbackData {
+    explicit ScreenCallbackData(ScreenCapturerMac* capturer)
+              : capturer(capturer) {}
+    rtc::CriticalSection crit_sect_;
+    ScreenCapturerMac* GUARDED_BY(crit_sect_) capturer;
+  };
+
+  ScreenCallbackData* screen_callback_data_;
 #endif
 
   std::unique_ptr<DesktopFrame> CreateFrame();
 
   Callback* callback_ = nullptr;
 
   CGLContextObj cgl_context_ = nullptr;
   ScopedPixelBufferObject pixel_buffer_object_;
@@ -412,26 +421,28 @@ class InvertedDesktopFrame : public Desk
  private:
   std::unique_ptr<DesktopFrame> original_frame_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(InvertedDesktopFrame);
 };
 
 ScreenCapturerMac::ScreenCapturerMac(
     rtc::scoped_refptr<DesktopConfigurationMonitor> desktop_config_monitor)
-    : desktop_config_monitor_(desktop_config_monitor) {
+    : screen_callback_data_(new ScreenCallbackData(this))
+    , desktop_config_monitor_(desktop_config_monitor) {
 #if defined(MAC_OS_X_VERSION_10_8) && \
   (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_8)
   display_stream_manager_ = new DisplayStreamManager;
 #endif
 }
 
 ScreenCapturerMac::~ScreenCapturerMac() {
   ReleaseBuffers();
-  UnregisterRefreshAndMoveHandlers();
+  rtc::CritScope lock(&screen_callback_data_->crit_sect_);
+  screen_callback_data_->capturer = nullptr;
 #if defined(MAC_OS_X_VERSION_10_8) && \
   (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_8)
   display_stream_manager_->PrepareForSelfDestruction();
 #endif
   dlclose(app_services_library_);
   dlclose(opengl_library_);
 }
 
@@ -1029,24 +1040,24 @@ bool ScreenCapturerMac::RegisterRefreshA
       CFRunLoopSourceRef source =
           CGDisplayStreamGetRunLoopSource(display_stream);
       CFRunLoopAddSource(CFRunLoopGetCurrent(), source, kCFRunLoopDefaultMode);
       display_stream_manager_->SaveStream(unique_id, display_stream);
     }
   }
 #else
  CGError err = CGRegisterScreenRefreshCallback(
-      ScreenCapturerMac::ScreenRefreshCallback, this);
+      ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data_);
   if (err != kCGErrorSuccess) {
     LOG(LS_ERROR) << "CGRegisterScreenRefreshCallback " << err;
     return false;
   }
 
   err = CGScreenRegisterMoveCallback(
-      ScreenCapturerMac::ScreenUpdateMoveCallback, this);
+      ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data_);
   if (err != kCGErrorSuccess) {
     LOG(LS_ERROR) << "CGScreenRegisterMoveCallback " << err;
     return false;
   }
 #endif
 
   return true;
 }
@@ -1093,31 +1104,53 @@ void ScreenCapturerMac::ScreenUpdateMove
 
   // Currently we just treat move events the same as refreshes.
   ScreenRefresh(count, refresh_rects);
 }
 
 void ScreenCapturerMac::ScreenRefreshCallback(CGRectCount count,
                                               const CGRect* rect_array,
                                               void* user_parameter) {
-  ScreenCapturerMac* capturer =
-      reinterpret_cast<ScreenCapturerMac*>(user_parameter);
-  if (capturer->screen_pixel_bounds_.is_empty())
-    capturer->ScreenConfigurationChanged();
-  capturer->ScreenRefresh(count, rect_array);
+  ScreenCallbackData* screen_callback_data =
+      reinterpret_cast<ScreenCallbackData*>(user_parameter);
+
+  rtc::CritScope lock(&screen_callback_data->crit_sect_);
+  if (!screen_callback_data->capturer) {
+    CGUnregisterScreenRefreshCallback(
+        ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data);
+    CGScreenUnregisterMoveCallback(
+        ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data);
+    delete screen_callback_data;
+    return;
+  }
+
+  if (screen_callback_data->capturer->screen_pixel_bounds_.is_empty())
+    screen_callback_data->capturer->ScreenConfigurationChanged();
+  screen_callback_data->capturer->ScreenRefresh(count, rect_array);
 }
 
 void ScreenCapturerMac::ScreenUpdateMoveCallback(
     CGScreenUpdateMoveDelta delta,
     size_t count,
     const CGRect* rect_array,
     void* user_parameter) {
-  ScreenCapturerMac* capturer =
-      reinterpret_cast<ScreenCapturerMac*>(user_parameter);
-  capturer->ScreenUpdateMove(delta, count, rect_array);
+  ScreenCallbackData* screen_callback_data =
+      reinterpret_cast<ScreenCallbackData*>(user_parameter);
+
+  rtc::CritScope lock(&screen_callback_data->crit_sect_);
+  if (!screen_callback_data->capturer) {
+    CGUnregisterScreenRefreshCallback(
+        ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data);
+    CGScreenUnregisterMoveCallback(
+        ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data);
+    delete screen_callback_data;
+    return;
+  }
+
+  screen_callback_data->capturer->ScreenUpdateMove(delta, count, rect_array);
 }
 #endif
 
 std::unique_ptr<DesktopFrame> ScreenCapturerMac::CreateFrame() {
   std::unique_ptr<DesktopFrame> frame(
       new BasicDesktopFrame(screen_pixel_bounds_.size()));
   frame->set_dpi(DesktopVector(kStandardDPI * dip_to_pixel_scale_,
                                kStandardDPI * dip_to_pixel_scale_));