Bug 1255823 - Add a two-step destruction process to PAPZ. r? draft
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 12 Apr 2016 11:16:01 -0400
changeset 350072 3a22bad90d32f9be38b568e31333c711433b373b
parent 349863 49d7fb650c9dde7cf6e4b2c7aa578a4a11e83f83
child 518238 c7c613354e9bf96e88dc4022d769f37ae994cdff
push id15234
push userkgupta@mozilla.com
push dateTue, 12 Apr 2016 18:31:46 +0000
bugs1255823
milestone48.0a1
Bug 1255823 - Add a two-step destruction process to PAPZ. r? MozReview-Commit-ID: 5eokgm6ufnF
gfx/layers/ipc/APZChild.cpp
gfx/layers/ipc/APZChild.h
gfx/layers/ipc/PAPZ.ipdl
gfx/layers/ipc/RemoteContentController.cpp
gfx/layers/ipc/RemoteContentController.h
--- 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);
@@ -304,44 +303,73 @@ RemoteContentController::RecvUpdateZoomC
 {
   if (RefPtr<APZCTreeManager> apzcTreeManager = GetApzcTreeManager()) {
     apzcTreeManager->UpdateZoomConstraints(ScrollableLayerGuid(mLayersId, aPresShellId, aViewId),
                                            aConstraints);
   }
   return true;
 }
 
+static void
+ReleaseDestroyedController(uint64_t aKey)
+{
+  // Clear the RefPtr in the sDestroyedControllers map in a runnable so that
+  // this object is destroyed after we unwind from the IPC code.
+  NS_DispatchToMainThread(NS_NewRunnableFunction([aKey]() {
+    sDestroyedControllers.erase(aKey);
+  }));
+}
+
+bool
+RemoteContentController::Recv__delete__()
+{
+  // This function is only ever called after we successfully call SendDestroy(),
+  // so we are guaranteed that sDestroyedControllers will contain this
+  // RemoteContentController.
+  ReleaseDestroyedController(mLayersId);
+  return true;
+}
+
 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 (aWhy == ActorDestroyReason::NormalShutdown) {
+    // Normal shutdown can happen after we call SendDestroy(), but before
+    // we Recv__delete__. In that case, the __delete__ message will never
+    // arrive, so we should release the controller from here. If this gets
+    // called before Destroy(), then this will be a no-op, and CanSend() will
+    // return false so the SendDestroy() will never happen.
+    ReleaseDestroyedController(mLayersId);
+  }
 }
 
 void
 RemoteContentController::Destroy()
 {
   RefPtr<RemoteContentController> controller = this;
   NS_DispatchToMainThread(NS_NewRunnableFunction([controller] {
-    if (controller->CanSend()) {
-      DeletePAPZParent(controller);
+    if (controller->CanSend() && controller->SendDestroy()) {
+      // Gfx code is done with this object, and it will probably get destroyed
+      // soon. We need to keep a RefPtr to this object until we get the
+      // __delete__ back, otherwise we might get destroyed in the meantime and
+      // the IPC code will crash on try to handle the __delete__.
+      uint64_t key = controller->mLayersId;
+      MOZ_ASSERT(sDestroyedControllers.find(key) == sDestroyedControllers.end());
+      sDestroyedControllers[key] = controller;
+    } else {
+      // If the send failed, then this object must already have had ActorDestroy
+      // called on it, and IPC code is no longer holding on to it, so we can
+      // let it get destroyed.
+      MOZ_ASSERT(controller->mBrowserParent == nullptr);
     }
   }));
 }
 
 void
 RemoteContentController::ChildAdopted()
 {
   // Clear the cached APZCTreeManager.
--- a/gfx/layers/ipc/RemoteContentController.h
+++ b/gfx/layers/ipc/RemoteContentController.h
@@ -90,16 +90,17 @@ public:
 
   virtual bool RecvSetAllowedTouchBehavior(const uint64_t& aInputBlockId,
                                            nsTArray<TouchBehaviorFlags>&& aFlags) override;
 
   virtual bool RecvUpdateZoomConstraints(const uint32_t& aPresShellId,
                                          const ViewID& aViewId,
                                          const MaybeZoomConstraints& aConstraints) override;
 
+  virtual bool Recv__delete__() override;
   virtual void ActorDestroy(ActorDestroyReason aWhy) override;
 
   virtual void Destroy() override;
 
   virtual void ChildAdopted() override;
 
 private:
   bool CanSend()