Bug 1553135 - Lock `LayerViewSupport` during detach so that other methods cannot be called during this time. r=snorp, a=RyanVM
authorEmily Toop <etoop@mozilla.com>
Wed, 24 Jul 2019 15:26:31 +0000
changeset 544776 ddfa84bcad9cc67afb25167c889c6ff28eea0d78
parent 544775 52440e49ce3b5da5ca765bfd9ac154ce5d92b453
child 544777 b04fe5054ebd5f4f0df4e25651e8243cfe92242b
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
bugs1553135
milestone69.0
Bug 1553135 - Lock `LayerViewSupport` during detach so that other methods cannot be called during this time. r=snorp, a=RyanVM This is caused by a race condition when the compositor is detached. Because the actual detachment happens in a new thread, `detach` can complete and release the lock on `mLayerViewSupport`, and `RecvScreenPixels` can obtain the lock, before `mLayerViewSupport` is properly cleaned up. We therefore check to ensure that `lvs` is not null before calling a method on it. Differential Revision: https://phabricator.services.mozilla.com/D36928
widget/android/nsWindow.cpp
--- a/widget/android/nsWindow.cpp
+++ b/widget/android/nsWindow.cpp
@@ -13,25 +13,25 @@
 
 #include "mozilla/MiscEvents.h"
 #include "mozilla/MouseEvents.h"
 #include "mozilla/TouchEvents.h"
 #include "mozilla/TypeTraits.h"
 #include "mozilla/WeakPtr.h"
 #include "mozilla/WheelHandlingHelper.h"  // for WheelDeltaAdjustmentStrategy
 
-#include "mozilla/a11y/SessionAccessibility.h"
-#include "mozilla/dom/ContentParent.h"
-#include "mozilla/dom/ContentChild.h"
-#include "mozilla/dom/MouseEventBinding.h"
+#include "mozilla/Preferences.h"
 #include "mozilla/Unused.h"
-#include "mozilla/Preferences.h"
+#include "mozilla/a11y/SessionAccessibility.h"
+#include "mozilla/dom/ContentChild.h"
+#include "mozilla/dom/ContentParent.h"
+#include "mozilla/dom/MouseEventBinding.h"
+#include "mozilla/gfx/2D.h"
+#include "mozilla/gfx/DataSurfaceHelpers.h"
 #include "mozilla/layers/RenderTrace.h"
-#include "mozilla/gfx/DataSurfaceHelpers.h"
-#include "mozilla/gfx/2D.h"
 #include <algorithm>
 
 using mozilla::Unused;
 using mozilla::dom::ContentChild;
 using mozilla::dom::ContentParent;
 
 #include "nsWindow.h"
 
@@ -48,56 +48,56 @@ using mozilla::dom::ContentParent;
 #include "nsIXULWindow.h"
 
 #include "nsAppShell.h"
 #include "nsFocusManager.h"
 #include "nsIdleService.h"
 #include "nsLayoutUtils.h"
 #include "nsViewManager.h"
 
+#include "WidgetUtils.h"
 #include "nsContentUtils.h"
-#include "WidgetUtils.h"
 
+#include "nsGfxCIID.h"
 #include "nsGkAtoms.h"
 #include "nsWidgetsCID.h"
-#include "nsGfxCIID.h"
 
 #include "gfxContext.h"
 
+#include "AndroidContentController.h"
+#include "GLContext.h"
+#include "GLContextProvider.h"
 #include "Layers.h"
-#include "mozilla/layers/LayerManagerComposite.h"
-#include "mozilla/layers/AsyncCompositionManager.h"
+#include "ScopedGLHelpers.h"
 #include "mozilla/layers/APZEventState.h"
 #include "mozilla/layers/APZInputBridge.h"
 #include "mozilla/layers/APZThreadUtils.h"
+#include "mozilla/layers/AsyncCompositionManager.h"
+#include "mozilla/layers/CompositorOGL.h"
 #include "mozilla/layers/IAPZCTreeManager.h"
-#include "GLContext.h"
-#include "GLContextProvider.h"
-#include "ScopedGLHelpers.h"
-#include "mozilla/layers/CompositorOGL.h"
-#include "AndroidContentController.h"
+#include "mozilla/layers/LayerManagerComposite.h"
 
 #include "nsTArray.h"
 
 #include "AndroidBridge.h"
 #include "AndroidBridgeUtilities.h"
 #include "AndroidUiThread.h"
 #include "FennecJNINatives.h"
+#include "GeckoEditableSupport.h"
 #include "GeneratedJNINatives.h"
-#include "GeckoEditableSupport.h"
 #include "KeyEvent.h"
 #include "MotionEvent.h"
 #include "ScreenHelperAndroid.h"
 
 #include "imgIEncoder.h"
 
-#include "nsString.h"
 #include "GeckoProfiler.h"  // For AUTO_PROFILER_LABEL
 #include "nsIXULRuntime.h"
 #include "nsPrintfCString.h"
+#include "nsString.h"
 
 #include "mozilla/ipc/Shmem.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 using namespace mozilla::layers;
 using namespace mozilla::java;
 using namespace mozilla::widget;
@@ -857,30 +857,37 @@ class nsWindow::LayerViewSupport final
 
   void OnDetach(already_AddRefed<Runnable> aDisposer) {
     if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) {
       GeckoSession::Compositor::GlobalRef compositor(mCompositor);
       if (!compositor) {
         return;
       }
 
-      uiThread->Dispatch(NS_NewRunnableFunction(
-          "LayerViewSupport::OnDetach",
-          [compositor, disposer = RefPtr<Runnable>(aDisposer),
-           result = &mCapturePixelsResults] {
-            while (!result->empty()) {
-              result->front()->CompleteExceptionally(
-                  java::sdk::IllegalStateException::New(
-                      "The compositor has detached from the session")
-                      .Cast<jni::Throwable>());
-              result->pop();
-            }
-            compositor->OnCompositorDetached();
-            disposer->Run();
-          }));
+      if (LockedWindowPtr window{mWindow}) {
+        uiThread->Dispatch(NS_NewRunnableFunction(
+            "LayerViewSupport::OnDetach",
+            [compositor, disposer = RefPtr<Runnable>(aDisposer),
+             results = &mCapturePixelsResults, lock = &window] {
+              if (lock) {
+                while (!results->empty()) {
+                  auto aResult = java::GeckoResult::LocalRef(results->front());
+                  if (aResult) {
+                    aResult->CompleteExceptionally(
+                        java::sdk::IllegalStateException::New(
+                            "The compositor has detached from the session")
+                            .Cast<jni::Throwable>());
+                    results->pop();
+                  }
+                }
+                compositor->OnCompositorDetached();
+                disposer->Run();
+              }
+            }));
+      }
     }
   }
 
   const GeckoSession::Compositor::Ref& GetJavaCompositor() const {
     return mCompositor;
   }
 
   bool CompositorPaused() const { return mCompositorPaused; }