Bug 1368030 - Fix race condition in ScreenCapturerMac. r=jesup
authorDan Minor <dminor@mozilla.com>
Wed, 19 Jul 2017 14:49:05 -0400
changeset 420052 baa75a4997524c032793ef508447ecc32bda622d
parent 420051 9ab04bf5afc543734878ebb305b295ca8720b36d
child 420053 28b09080d79ac444361f5d9f2e3c851ba9ec97f8
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* 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,30 @@ 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 +1042,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 +1106,57 @@ 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);
+
+  screen_callback_data->crit_sect_.Enter();
+  if (!screen_callback_data->capturer) {
+    CGUnregisterScreenRefreshCallback(
+        ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data);
+    CGScreenUnregisterMoveCallback(
+        ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data);
+    screen_callback_data->crit_sect_.Leave();
+    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);
+  screen_callback_data->crit_sect_.Leave();
 }
 
 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);
+
+  screen_callback_data->crit_sect_.Enter();
+  if (!screen_callback_data->capturer) {
+    CGUnregisterScreenRefreshCallback(
+        ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data);
+    CGScreenUnregisterMoveCallback(
+        ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data);
+    screen_callback_data->crit_sect_.Leave();
+    delete screen_callback_data;
+    return;
+  }
+
+  screen_callback_data->capturer->ScreenUpdateMove(delta, count, rect_array);
+  screen_callback_data->crit_sect_.Leave();
 }
 #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_));