Bug 1255823 - Add a two-step destruction process to PAPZ. r=dvander
authorKartikaya Gupta <kgupta@mozilla.com>
Mon, 18 Apr 2016 17:25:25 -0400
changeset 317445 e9500cac731dcc7456eee99ea16c9163a9c40b34
parent 317444 cc424788afd810c91f5f1420f6ab8729b94faca1
child 317446 865e124224cd54ec70adbefbcb8a750f5e1d985c
push id9480
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 17:12:58 +0000
treeherdermozilla-aurora@0d6a91c76a9e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander
bugs1255823
milestone48.0a1
Bug 1255823 - Add a two-step destruction process to PAPZ. r=dvander MozReview-Commit-ID: FuiwoIAZUKj
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);
@@ -138,30 +143,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
@@ -43,24 +43,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,25 +78,27 @@ 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 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);
@@ -294,36 +293,40 @@ 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);
+  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.