Bug 1368030 - Fix race condition in ScreenCapturerMac. r=jesup, a=jcristau
authorDan Minor <dminor@mozilla.com>
Tue, 25 Jul 2017 11:06:19 -0400
changeset 414470 60f41b3decf8b73346d6641ebb0449ed7bfbab20
parent 414469 735648f1fd7f92c42f34666e7b66475b04d5bb32
child 414471 459b23d30228b91a54aa955cf65c907b3a798297
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, jcristau
bugs1368030
milestone55.0
Bug 1368030 - Fix race condition in ScreenCapturerMac. r=jesup, a=jcristau
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
@@ -16,16 +16,17 @@
 #include <ApplicationServices/ApplicationServices.h>
 #include <Cocoa/Cocoa.h>
 #include <dlfcn.h>
 #include <IOKit/pwr_mgt/IOPMLib.h>
 #include <OpenGL/CGLMacro.h>
 #include <OpenGL/OpenGL.h>
 
 #include "webrtc/base/macutils.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/scoped_ptr.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"
 #include "webrtc/modules/desktop_capture/mac/scoped_pixel_buffer_object.h"
@@ -227,16 +228,25 @@ class ScreenCapturerMac : public ScreenC
                                     const CGRect *rect_array,
                                     void *user_parameter);
   static void ScreenUpdateMoveCallback(CGScreenUpdateMoveDelta delta,
                                        size_t count,
                                        const CGRect *rect_array,
                                        void *user_parameter);
   void ReleaseBuffers();
 
+  struct ScreenCallbackData {
+    explicit ScreenCallbackData(ScreenCapturerMac* capturer)
+              : capturer(capturer) {}
+    rtc::CriticalSection crit_sect_;
+    ScreenCapturerMac* GUARDED_BY(crit_sect_) capturer;
+  };
+
+  ScreenCallbackData* screen_callback_data_;
+
   DesktopFrame* CreateFrame();
 
   Callback* callback_;
 
   CGLContextObj cgl_context_;
   ScopedPixelBufferObject pixel_buffer_object_;
 
   // Queue of the frames buffers.
@@ -304,17 +314,18 @@ class InvertedDesktopFrame : public Desk
  private:
   rtc::scoped_ptr<DesktopFrame> original_frame_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(InvertedDesktopFrame);
 };
 
 ScreenCapturerMac::ScreenCapturerMac(
     rtc::scoped_refptr<DesktopConfigurationMonitor> desktop_config_monitor)
-    : callback_(NULL),
+    : screen_callback_data_(new ScreenCallbackData(this)),
+      callback_(NULL),
       cgl_context_(NULL),
       current_display_(0),
       dip_to_pixel_scale_(1.0f),
       desktop_config_monitor_(desktop_config_monitor),
       power_assertion_id_display_(kIOPMNullAssertionID),
       power_assertion_id_user_(kIOPMNullAssertionID),
       app_services_library_(NULL),
       cg_display_base_address_(NULL),
@@ -331,17 +342,20 @@ ScreenCapturerMac::~ScreenCapturerMac() 
     power_assertion_id_display_ = kIOPMNullAssertionID;
   }
   if (power_assertion_id_user_ != kIOPMNullAssertionID) {
     IOPMAssertionRelease(power_assertion_id_user_);
     power_assertion_id_user_ = kIOPMNullAssertionID;
   }
 
   ReleaseBuffers();
-  UnregisterRefreshAndMoveHandlers();
+  {
+    rtc::CritScope lock(&screen_callback_data_->crit_sect_);
+    screen_callback_data_->capturer = nullptr;
+  }
   dlclose(app_services_library_);
   dlclose(opengl_library_);
 }
 
 bool ScreenCapturerMac::Init() {
   if (!RegisterRefreshAndMoveHandlers()) {
     return false;
   }
@@ -890,37 +904,37 @@ void ScreenCapturerMac::ScreenConfigurat
   size_t buffer_size = screen_pixel_bounds_.width() *
                        screen_pixel_bounds_.height() *
                        sizeof(uint32_t);
   pixel_buffer_object_.Init(cgl_context_, buffer_size);
 }
 
 bool ScreenCapturerMac::RegisterRefreshAndMoveHandlers() {
   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;
   }
 
   return true;
 }
 
 void ScreenCapturerMac::UnregisterRefreshAndMoveHandlers() {
   CGUnregisterScreenRefreshCallback(
-      ScreenCapturerMac::ScreenRefreshCallback, this);
+      ScreenCapturerMac::ScreenRefreshCallback, screen_callback_data_);
   CGScreenUnregisterMoveCallback(
-      ScreenCapturerMac::ScreenUpdateMoveCallback, this);
+      ScreenCapturerMac::ScreenUpdateMoveCallback, screen_callback_data_);
 }
 
 void ScreenCapturerMac::ScreenRefresh(CGRectCount count,
                                       const CGRect* rect_array) {
   if (screen_pixel_bounds_.is_empty())
     return;
 
   DesktopRegion region;
@@ -948,31 +962,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();
 }
 
 DesktopFrame* ScreenCapturerMac::CreateFrame() {
   rtc::scoped_ptr<DesktopFrame> frame(
       new BasicDesktopFrame(screen_pixel_bounds_.size()));
 
   frame->set_dpi(DesktopVector(kStandardDPI * dip_to_pixel_scale_,
                                kStandardDPI * dip_to_pixel_scale_));