Bug 1560641: Add a lock around `mCapturePixelsResults`. r=snorp, a=RyanVM
authorEmily Toop <etoop@mozilla.com>
Wed, 24 Jul 2019 16:23:30 +0000
changeset 544777 b04fe5054ebd5f4f0df4e25651e8243cfe92242b
parent 544776 ddfa84bcad9cc67afb25167c889c6ff28eea0d78
child 544778 383500a28e6e5b19c7177a63c2837b815768363f
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssnorp, RyanVM
bugs1560641
milestone69.0
Bug 1560641: Add a lock around `mCapturePixelsResults`. r=snorp, a=RyanVM This is to ensure that multiple completions cannot be attempted on the same `GeckoResult`, resulting in crashes. Differential Revision: https://phabricator.services.mozilla.com/D36929
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt
widget/android/nsWindow.cpp
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt
@@ -96,21 +96,9 @@ class ScreenshotTest : BaseSessionTest()
             val result = it.capturePixels()
             val texture = SurfaceTexture(0)
             texture.setDefaultBufferSize(SCREEN_WIDTH, SCREEN_HEIGHT)
             val surface = Surface(texture)
             it.surfaceChanged(surface, SCREEN_WIDTH, SCREEN_HEIGHT)
             sessionRule.waitForResult(result)
         }
     }
-
-    @Ignore //Disable test for frequent failures Bug 1557569
-    @Test
-    fun capturePixelsThrowsCompositorNotReady() {
-        expectedEx.expect(IllegalStateException::class.java)
-        expectedEx.expectMessage("Compositor must be ready before pixels can be captured")
-        val session = sessionRule.createClosedSession()
-        val display = session.acquireDisplay()
-
-        sessionRule.waitForResult(display.capturePixels())
-        fail("IllegalStateException expected to be thrown")
-    }
 }
--- a/widget/android/nsWindow.cpp
+++ b/widget/android/nsWindow.cpp
@@ -8,18 +8,21 @@
 #include <android/native_window.h>
 #include <android/native_window_jni.h>
 #include <math.h>
 #include <queue>
 #include <unistd.h>
 
 #include "mozilla/MiscEvents.h"
 #include "mozilla/MouseEvents.h"
+#include "mozilla/Preferences.h"
+#include "mozilla/RWLock.h"
 #include "mozilla/TouchEvents.h"
 #include "mozilla/TypeTraits.h"
+#include "mozilla/Unused.h"
 #include "mozilla/WeakPtr.h"
 #include "mozilla/WheelHandlingHelper.h"  // for WheelDeltaAdjustmentStrategy
 
 #include "mozilla/Preferences.h"
 #include "mozilla/Unused.h"
 #include "mozilla/a11y/SessionAccessibility.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ContentParent.h"
@@ -1127,46 +1130,60 @@ class nsWindow::LayerViewSupport final
     if (RefPtr<UiCompositorControllerChild> child =
             GetUiCompositorControllerChild()) {
       child->SetDefaultClearColor((uint32_t)aColor);
     }
   }
 
   void RequestScreenPixels(jni::Object::Param aResult) {
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
-    mCapturePixelsResults.push(
-        java::GeckoResult::GlobalRef(java::GeckoResult::LocalRef(aResult)));
-    if (mCapturePixelsResults.size() == 1) {
+
+    int size = 0;
+    if (LockedWindowPtr window{mWindow}) {
+      mCapturePixelsResults.push(
+          java::GeckoResult::GlobalRef(java::GeckoResult::LocalRef(aResult)));
+      size = mCapturePixelsResults.size();
+    }
+
+    if (size == 1) {
       if (RefPtr<UiCompositorControllerChild> child =
               GetUiCompositorControllerChild()) {
         child->RequestScreenPixels();
       }
     }
   }
 
   void RecvScreenPixels(Shmem&& aMem, const ScreenIntSize& aSize) {
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
-    auto aResult = java::GeckoResult::LocalRef(mCapturePixelsResults.front());
+    java::GeckoResult::LocalRef aResult = nullptr;
+    if (LockedWindowPtr window{mWindow}) {
+      aResult = java::GeckoResult::LocalRef(mCapturePixelsResults.front());
+    }
     if (aResult) {
       auto pixels = mozilla::jni::ByteBuffer::New(FlipScreenPixels(aMem, aSize),
                                                   aMem.Size<int8_t>());
       auto bitmap = java::sdk::Bitmap::CreateBitmap(
           aSize.width, aSize.height, java::sdk::Config::ARGB_8888());
       bitmap->CopyPixelsFromBuffer(pixels);
       aResult->Complete(bitmap);
-      mCapturePixelsResults.pop();
+
+      if (LockedWindowPtr window{mWindow}) {
+        mCapturePixelsResults.pop();
+      }
     }
 
     // Pixels have been copied, so Dealloc Shmem
     if (RefPtr<UiCompositorControllerChild> child =
             GetUiCompositorControllerChild()) {
       child->DeallocPixelBuffer(aMem);
 
-      if (!mCapturePixelsResults.empty()) {
-        child->RequestScreenPixels();
+      if (LockedWindowPtr window{mWindow}) {
+        if (!mCapturePixelsResults.empty()) {
+          child->RequestScreenPixels();
+        }
       }
     }
   }
 
   void EnableLayerUpdateNotifications(bool aEnable) {
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
     if (RefPtr<UiCompositorControllerChild> child =
             GetUiCompositorControllerChild()) {