Bug 1617523 - Ensure remote backbuffer only reused if initialized properly r=jrmuizel
authorChris Martin <cmartin@mozilla.com>
Fri, 28 Feb 2020 01:34:26 +0000
changeset 516059 992f5734eb4e39b3c76ff35b0f744b129f80b483
parent 516058 133376007dced39abb6a2938a30862c3c5c7a0ec
child 516060 7f6eb10a65170250ec5ce8b1bb6bebc8195c7be4
push id37165
push useraiakab@mozilla.com
push dateFri, 28 Feb 2020 09:24:28 +0000
treeherdermozilla-central@fb4f281c1c54 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1617523
milestone75.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 1617523 - Ensure remote backbuffer only reused if initialized properly r=jrmuizel This crash was caused because there was a case where the Provider would store the SharedImage even if it failed to initialize it or send it to the Client. When the Client next requested a borrow, the Provider would see it was already storing a SharedImage and would tell the Client to just reuse the one it already had. Since the Client never actually received it, it would end up dereferencing a null pointer. The fix for this is to wait until the SharedImage is fully initialized and shared with the Client before storing it. That way, we know that if we have it then so does the Client. Differential Revision: https://phabricator.services.mozilla.com/D64659
widget/windows/RemoteBackbuffer.cpp
widget/windows/RemoteBackbuffer.h
--- a/widget/windows/RemoteBackbuffer.cpp
+++ b/widget/windows/RemoteBackbuffer.cpp
@@ -14,16 +14,17 @@ enum class ResponseResult {
   Error,
   BorrowSuccess,
   BorrowSameBuffer,
   PresentSuccess
 };
 
 enum class SharedDataType {
   BorrowRequest,
+  BorrowRequestAllowSameBuffer,
   BorrowResponse,
   PresentRequest,
   PresentResponse
 };
 
 struct BorrowResponseData {
   ResponseResult result;
   int32_t width;
@@ -381,20 +382,23 @@ void Provider::ThreadMain() {
     MOZ_ALWAYS_TRUE(::WaitForSingleObject(mRequestReadyEvent, INFINITE) ==
                     WAIT_OBJECT_0);
 
     if (mStopServiceThread) {
       break;
     }
 
     switch (mSharedDataPtr->dataType) {
-      case SharedDataType::BorrowRequest: {
+      case SharedDataType::BorrowRequest:
+      case SharedDataType::BorrowRequestAllowSameBuffer: {
         BorrowResponseData responseData = {};
 
-        HandleBorrowRequest(&responseData);
+        HandleBorrowRequest(&responseData,
+                            mSharedDataPtr->dataType ==
+                                SharedDataType::BorrowRequestAllowSameBuffer);
 
         mSharedDataPtr->dataType = SharedDataType::BorrowResponse;
         mSharedDataPtr->data.borrowResponse = responseData;
 
         MOZ_ALWAYS_TRUE(::SetEvent(mResponseReadyEvent));
 
         break;
       }
@@ -411,57 +415,61 @@ void Provider::ThreadMain() {
         break;
       }
       default:
         break;
     };
   }
 }
 
-void Provider::HandleBorrowRequest(BorrowResponseData* aResponseData) {
+void Provider::HandleBorrowRequest(BorrowResponseData* aResponseData,
+                                   bool aAllowSameBuffer) {
   MOZ_ASSERT(aResponseData);
 
   aResponseData->result = ResponseResult::Error;
 
   RECT clientRect = {};
   if (!::GetClientRect(mWindowHandle, &clientRect)) {
     return;
   }
 
   MOZ_ASSERT(clientRect.left == 0);
   MOZ_ASSERT(clientRect.top == 0);
 
   int32_t width = clientRect.right ? clientRect.right : 1;
   int32_t height = clientRect.bottom ? clientRect.bottom : 1;
 
-  bool needNewBackbuffer = !mBackbuffer || (mBackbuffer->GetWidth() != width) ||
+  bool needNewBackbuffer = !aAllowSameBuffer || !mBackbuffer ||
+                           (mBackbuffer->GetWidth() != width) ||
                            (mBackbuffer->GetHeight() != height);
 
   if (!needNewBackbuffer) {
     aResponseData->result = ResponseResult::BorrowSameBuffer;
     return;
   }
 
   mBackbuffer.reset();
 
-  mBackbuffer = std::make_unique<PresentableSharedImage>();
-  if (!mBackbuffer->Initialize(width, height)) {
+  auto newBackbuffer = std::make_unique<PresentableSharedImage>();
+  if (!newBackbuffer->Initialize(width, height)) {
     return;
   }
 
   HANDLE remoteFileMapping =
-      mBackbuffer->CreateRemoteFileMapping(mTargetProcessId);
+      newBackbuffer->CreateRemoteFileMapping(mTargetProcessId);
   if (!remoteFileMapping) {
     return;
   }
 
   aResponseData->result = ResponseResult::BorrowSuccess;
   aResponseData->width = width;
   aResponseData->height = height;
   aResponseData->fileMapping = remoteFileMapping;
+
+  mBackbuffer = std::move(newBackbuffer);
 }
 
 void Provider::HandlePresentRequest(PresentResponseData* aResponseData) {
   MOZ_ASSERT(aResponseData);
 
   aResponseData->result = ResponseResult::Error;
 
   if (!mBackbuffer) {
@@ -521,17 +529,20 @@ bool Client::Initialize(const RemoteBack
   }
 
   mSharedDataPtr = reinterpret_cast<SharedData*>(mappedFilePtr);
 
   return true;
 }
 
 already_AddRefed<gfx::DrawTarget> Client::BorrowDrawTarget() {
-  mSharedDataPtr->dataType = SharedDataType::BorrowRequest;
+  mSharedDataPtr->dataType = mBackbuffer
+                                 ? SharedDataType::BorrowRequestAllowSameBuffer
+                                 : SharedDataType::BorrowRequest;
+
   MOZ_ALWAYS_TRUE(::SetEvent(mRequestReadyEvent));
   MOZ_ALWAYS_TRUE(::WaitForSingleObject(mResponseReadyEvent, INFINITE) ==
                   WAIT_OBJECT_0);
 
   if (mSharedDataPtr->dataType != SharedDataType::BorrowResponse) {
     return nullptr;
   }
 
@@ -540,23 +551,28 @@ already_AddRefed<gfx::DrawTarget> Client
   if ((responseData.result != ResponseResult::BorrowSameBuffer) &&
       (responseData.result != ResponseResult::BorrowSuccess)) {
     return nullptr;
   }
 
   if (responseData.result == ResponseResult::BorrowSuccess) {
     mBackbuffer.reset();
 
-    mBackbuffer = std::make_unique<SharedImage>();
-    if (!mBackbuffer->InitializeRemote(responseData.width, responseData.height,
-                                       responseData.fileMapping)) {
+    auto newBackbuffer = std::make_unique<SharedImage>();
+    if (!newBackbuffer->InitializeRemote(responseData.width,
+                                         responseData.height,
+                                         responseData.fileMapping)) {
       return nullptr;
     }
+
+    mBackbuffer = std::move(newBackbuffer);
   }
 
+  MOZ_ASSERT(mBackbuffer);
+
   return mBackbuffer->CreateDrawTarget();
 }
 
 bool Client::PresentDrawTarget() {
   mSharedDataPtr->dataType = SharedDataType::PresentRequest;
   MOZ_ALWAYS_TRUE(::SetEvent(mRequestReadyEvent));
   MOZ_ALWAYS_TRUE(::WaitForSingleObject(mResponseReadyEvent, INFINITE) ==
                   WAIT_OBJECT_0);
--- a/widget/windows/RemoteBackbuffer.h
+++ b/widget/windows/RemoteBackbuffer.h
@@ -35,17 +35,18 @@ class Provider {
   Provider(const Provider&) = delete;
   Provider(Provider&&) = delete;
   Provider& operator=(const Provider&) = delete;
   Provider& operator=(Provider&&) = delete;
 
  private:
   void ThreadMain();
 
-  void HandleBorrowRequest(BorrowResponseData* aResponseData);
+  void HandleBorrowRequest(BorrowResponseData* aResponseData,
+                           bool aAllowSameBuffer);
   void HandlePresentRequest(PresentResponseData* aResponseData);
 
   HWND mWindowHandle;
   DWORD mTargetProcessId;
   HANDLE mFileMapping;
   HANDLE mRequestReadyEvent;
   HANDLE mResponseReadyEvent;
   SharedData* mSharedDataPtr;