Bug 1255823 - Add a two-step destruction process to PAPZ. r=dvander
☠☠ backed out by 6e6c5143feaa ☠ ☠
authorKartikaya Gupta <kgupta@mozilla.com>
Mon, 18 Apr 2016 10:15:00 -0400
changeset 331479 6d973d2f1bae985278756e086e7e8b5c094d0806
parent 331478 b1da60432e4188749a6b961bbb7ed1900433376a
child 331480 77279d5587f2402ac9b24fde74d1cfacd29ad6bb
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander
bugs1255823
milestone48.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 1255823 - Add a two-step destruction process to PAPZ. r=dvander MozReview-Commit-ID: E6dyipItPSm
gfx/layers/ipc/APZChild.cpp
gfx/layers/ipc/APZChild.h
gfx/layers/ipc/PAPZ.ipdl
gfx/layers/ipc/RemoteContentController.cpp
--- a/gfx/layers/ipc/APZChild.cpp
+++ b/gfx/layers/ipc/APZChild.cpp
@@ -73,22 +73,27 @@ APZChild::Create(const dom::TabId& aTabI
       return nullptr;
     }
     apz->SetObserver(observer);
   }
 
   return apz.forget();
 }
 
+APZChild::APZChild()
+  : mDestroyed(false)
+{
+}
+
 APZChild::~APZChild()
 {
   if (mObserver) {
     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
     os->RemoveObserver(mObserver, "tab-child-created");
-  } else {
+  } else if (mBrowser) {
     mBrowser->SetAPZChild(nullptr);
   }
 }
 
 bool
 APZChild::RecvUpdateFrame(const FrameMetrics& aFrameMetrics)
 {
   return mBrowser->UpdateFrame(aFrameMetrics);
@@ -146,30 +151,47 @@ APZChild::RecvNotifyFlushComplete()
   nsCOMPtr<nsIPresShell> shell;
   if (nsCOMPtr<nsIDocument> doc = mBrowser->GetDocument()) {
     shell = doc->GetShell();
   }
   APZCCallbackHelper::NotifyFlushComplete(shell.get());
   return true;
 }
 
+bool
+APZChild::RecvDestroy()
+{
+  mDestroyed = true;
+  if (mBrowser) {
+    mBrowser->SetAPZChild(nullptr);
+    mBrowser = nullptr;
+  }
+  PAPZChild::Send__delete__(this);
+  return true;
+}
+
 void
 APZChild::SetObserver(nsIObserver* aObserver)
 {
   MOZ_ASSERT(!mBrowser);
   mObserver = aObserver;
 }
 
 void
 APZChild::SetBrowser(dom::TabChild* aBrowser)
 {
   MOZ_ASSERT(!mBrowser);
   if (mObserver) {
     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
     os->RemoveObserver(mObserver, "tab-child-created");
     mObserver = nullptr;
   }
-  mBrowser = aBrowser;
-  mBrowser->SetAPZChild(this);
+  // We might get the tab-child-created notification after we receive a
+  // Destroy message from the parent. In that case we don't want to install
+  // ourselves with the browser.
+  if (!mDestroyed) {
+    mBrowser = aBrowser;
+    mBrowser->SetAPZChild(this);
+  }
 }
 
 } // namespace layers
 } // namespace mozilla
--- a/gfx/layers/ipc/APZChild.h
+++ b/gfx/layers/ipc/APZChild.h
@@ -46,24 +46,27 @@ public:
                                  const uint64_t& aInputBlockId) override;
 
   virtual bool RecvNotifyAPZStateChange(const ViewID& aViewId,
                                         const APZStateChange& aChange,
                                         const int& aArg) override;
 
   virtual bool RecvNotifyFlushComplete() override;
 
+  virtual bool RecvDestroy() override;
+
   void SetBrowser(dom::TabChild* aBrowser);
 
 private:
-  APZChild() {};
+  APZChild();
 
   void SetObserver(nsIObserver* aObserver);
 
   RefPtr<dom::TabChild> mBrowser;
   RefPtr<nsIObserver> mObserver;
+  bool mDestroyed;
 };
 
 } // namespace layers
 
 } // namespace mozilla
 
 #endif // mozilla_layers_APZChild_h
--- a/gfx/layers/ipc/PAPZ.ipdl
+++ b/gfx/layers/ipc/PAPZ.ipdl
@@ -78,26 +78,28 @@ parent:
   /**
    * Updates the zoom constraints for a scrollable frame in this tab.
    * The zoom controller code lives on the parent side and so this allows it to
    * have up-to-date zoom constraints.
    */
   async UpdateZoomConstraints(uint32_t aPresShellId, ViewID aViewId,
                               MaybeZoomConstraints aConstraints);
 
+  async __delete__();
+
 child:
   async UpdateFrame(FrameMetrics frame);
 
   // The following methods correspond to functions on the GeckoContentController
   // interface in gfx/layers/apz/public/GeckoContentController.h. Refer to documentation
   // in that file for these functions.
   async AcknowledgeScrollUpdate(ViewID aScrollId, uint32_t aScrollGeneration);
   async HandleDoubleTap(CSSPoint aPoint, Modifiers aModifiers, ScrollableLayerGuid aGuid);
   async HandleSingleTap(CSSPoint aPoint, Modifiers aModifiers, ScrollableLayerGuid aGuid, bool aCallTakeFocusForClickFromTap);
   async HandleLongTap(CSSPoint point, Modifiers aModifiers, ScrollableLayerGuid aGuid, uint64_t aInputBlockId);
   async NotifyAPZStateChange(ViewID aViewId, APZStateChange aChange, int aArg);
   async NotifyFlushComplete();
 
-  async __delete__();
+  async Destroy();
 };
 
 } // layers
 } // mozilla
--- a/gfx/layers/ipc/RemoteContentController.cpp
+++ b/gfx/layers/ipc/RemoteContentController.cpp
@@ -20,31 +20,30 @@
 #include "Units.h"
 #ifdef MOZ_WIDGET_ANDROID
 #include "AndroidBridge.h"
 #endif
 
 namespace mozilla {
 namespace layers {
 
+static std::map<uint64_t, RefPtr<RemoteContentController>> sDestroyedControllers;
+
 RemoteContentController::RemoteContentController(uint64_t aLayersId,
                                                  dom::TabParent* aBrowserParent)
   : mUILoop(MessageLoop::current())
   , mLayersId(aLayersId)
   , mBrowserParent(aBrowserParent)
   , mMutex("RemoteContentController")
 {
   MOZ_ASSERT(NS_IsMainThread());
 }
 
 RemoteContentController::~RemoteContentController()
 {
-  if (mBrowserParent) {
-    Unused << PAPZParent::Send__delete__(this);
-  }
 }
 
 void
 RemoteContentController::RequestContentRepaint(const FrameMetrics& aFrameMetrics)
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (CanSend()) {
     Unused << SendUpdateFrame(aFrameMetrics);
@@ -312,36 +311,47 @@ RemoteContentController::RecvUpdateZoomC
 void
 RemoteContentController::ActorDestroy(ActorDestroyReason aWhy)
 {
   {
     MutexAutoLock lock(mMutex);
     mApzcTreeManager = nullptr;
   }
   mBrowserParent = nullptr;
-}
 
-// TODO: Remove once upgraded to GCC 4.8+ on linux. Calling a static member
-//       function (like PAPZParent::Send__delete__) in a lambda leads to a bogus
-//       error: "'this' was not captured for this lambda function".
-//
-//       (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51494)
-static void
-DeletePAPZParent(PAPZParent* aPAPZ)
-{
-  Unused << PAPZParent::Send__delete__(aPAPZ);
+  // If this gets called with AbnormalShutdown, there might still be a __delete__
+  // message coming in, and we can't free this object until that is received, or
+  // the IPC code will crash trying to call Recv__delete__.
+  // For all other values of aWhy we should be safe to free the object here,
+  // because we don't expect any other IPC messages to be coming in.
+  if (aWhy != ActorDestroyReason::AbnormalShutdown) {
+    uint64_t key = mLayersId;
+    NS_DispatchToMainThread(NS_NewRunnableFunction([key]() {
+      // sDestroyedControllers may or may not contain the key, depending on
+      // whether or not SendDestroy() was successfully sent out or not.
+      sDestroyedControllers.erase(key);
+    }));
+  }
 }
 
 void
 RemoteContentController::Destroy()
 {
   RefPtr<RemoteContentController> controller = this;
   NS_DispatchToMainThread(NS_NewRunnableFunction([controller] {
     if (controller->CanSend()) {
-      DeletePAPZParent(controller);
+      // Gfx code is done with this object, and it will probably get destroyed
+      // soon. However, if CanSend() is true, ActorDestroy has not yet been
+      // called, which means IPC code still has a handle to this object. We need
+      // to keep it alive until we get the ActorDestroy call, either via the
+      // __delete__ message or via IPC shutdown on our end.
+      uint64_t key = controller->mLayersId;
+      MOZ_ASSERT(sDestroyedControllers.find(key) == sDestroyedControllers.end());
+      sDestroyedControllers[key] = controller;
+      Unused << controller->SendDestroy();
     }
   }));
 }
 
 void
 RemoteContentController::ChildAdopted()
 {
   // Clear the cached APZCTreeManager.