Bug 1255823 - Add a two-step destruction process to PAPZ. r=dvander a=ritu

MozReview-Commit-ID: FuiwoIAZUKj

gfx/layers/ipc/APZChild.cpp

@@ -78,12 +78,17 @@ APZChild::Create(const dom::TabId& aTabId)
   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);
   }
 }
@@ -159,6 +164,18 @@ APZChild::RecvNotifyFlushComplete()
   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)
 {
@@ -175,8 +192,13 @@ APZChild::SetBrowser(dom::TabChild* aBrowser)
     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

gfx/layers/ipc/APZChild.h

@@ -54,15 +54,18 @@ public:
 
   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

gfx/layers/ipc/PAPZ.ipdl

@@ -83,6 +83,8 @@ parent:
   async UpdateZoomConstraints(uint32_t aPresShellId, ViewID aViewId,
                               MaybeZoomConstraints aConstraints);
 
+  async __delete__();
+
 child:
   async UpdateFrame(FrameMetrics frame);
 
@@ -97,7 +99,7 @@ child:
   async NotifyAPZStateChange(ViewID aViewId, APZStateChange aChange, int aArg);
   async NotifyFlushComplete();
 
-  async __delete__();
+  async Destroy();
 };
 
 } // layers

gfx/layers/ipc/RemoteContentController.cpp

@@ -25,6 +25,8 @@
 namespace mozilla {
 namespace layers {
 
+static std::map<uint64_t, RefPtr<RemoteContentController>> sDestroyedControllers;
+
 RemoteContentController::RemoteContentController(uint64_t aLayersId,
                                                  dom::TabParent* aBrowserParent)
   : mUILoop(MessageLoop::current())
@@ -37,9 +39,6 @@ RemoteContentController::RemoteContentController(uint64_t aLayersId,
 
 RemoteContentController::~RemoteContentController()
 {
-  if (mBrowserParent) {
-    Unused << PAPZParent::Send__delete__(this);
-  }
 }
 
 void
@@ -335,17 +334,13 @@ RemoteContentController::ActorDestroy(ActorDestroyReason aWhy)
     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
@@ -354,7 +349,15 @@ 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();
     }
   }));
 }