Bug 1656854 - Part 2: Add a BrowsingContextGroup keepalive to BrowserParent shutdown, r=farre
authorNika Layzell <nika@thelayzells.com>
Thu, 06 Aug 2020 14:04:13 +0000
changeset 544142 ee09cb88af177571304759bc05c760e3d82fd1ed
parent 544141 d5f1c00144e2e425c9c965a7798b15c053447be0
child 544143 600b0c60d674ecad1372a9a34b83baf163ff259f
push id123840
push usernlayzell@mozilla.com
push dateMon, 10 Aug 2020 18:10:01 +0000
treeherderautoland@ee09cb88af17 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfarre
bugs1656854, 1652085
milestone81.0a1
first release with
nightly linux32
ee09cb88af17 / 81.0a1 / 20200810213634 / files
nightly linux64
ee09cb88af17 / 81.0a1 / 20200810213634 / files
nightly mac
ee09cb88af17 / 81.0a1 / 20200810213634 / files
nightly win32
ee09cb88af17 / 81.0a1 / 20200810213634 / files
nightly win64
ee09cb88af17 / 81.0a1 / 20200810213634 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1656854 - Part 2: Add a BrowsingContextGroup keepalive to BrowserParent shutdown, r=farre In bug 1652085, I added BrowsingContextGroup keepalives while waiting for replies to the discard message, however that message isn't actually sent to the current owner process. Instead, the BrowsingContext is discarded by the PBrowser being destroyed. This should help ensure we also keep the group alive during normal BrowserParent destruction. Differential Revision: https://phabricator.services.mozilla.com/D85897
dom/ipc/BrowserParent.cpp
--- a/dom/ipc/BrowserParent.cpp
+++ b/dom/ipc/BrowserParent.cpp
@@ -628,16 +628,24 @@ void BrowserParent::Destroy() {
   }
 
   DestroyInternal();
 
   mIsDestroyed = true;
 
   Manager()->NotifyTabDestroying();
 
+  // This `AddKeepAlive` will be cleared if `mMarkedDestroying` is set in
+  // `ActorDestroy`. Out of caution, we don't add the `KeepAlive` if our IPC
+  // actor has somehow already been destroyed, as that would mean `ActorDestroy`
+  // won't be called.
+  if (CanRecv()) {
+    mBrowsingContext->Group()->AddKeepAlive();
+  }
+
   mMarkedDestroying = true;
 }
 
 mozilla::ipc::IPCResult BrowserParent::RecvEnsureLayersConnected(
     CompositorOptions* aCompositorOptions) {
   if (mRemoteLayerTreeOwner.IsInitialized()) {
     mRemoteLayerTreeOwner.EnsureLayersConnected(aCompositorOptions);
   }
@@ -701,16 +709,23 @@ void BrowserParent::ActorDestroy(ActorDe
   // there is no need to call ContentParent::NotifyTabDestroying
   // because the jobs in ContentParent::NotifyTabDestroying
   // will be done by ContentParent::ActorDestroy.
   if (XRE_IsContentProcess() && why == AbnormalShutdown && !mIsDestroyed) {
     DestroyInternal();
     mIsDestroyed = true;
   }
 
+  // If we were shutting down normally, we held a reference to our
+  // BrowsingContextGroup in `BrowserParent::Destroy`. Clear that reference
+  // here.
+  if (mMarkedDestroying) {
+    mBrowsingContext->Group()->RemoveKeepAlive();
+  }
+
   // Tell our embedder that the tab is now going away unless we're an
   // out-of-process iframe.
   RefPtr<nsFrameLoader> frameLoader = GetFrameLoader(true);
   if (frameLoader) {
     ReceiveMessage(CHILD_PROCESS_SHUTDOWN_MESSAGE, false, nullptr);
 
     if (mBrowsingContext->IsTop()) {
       // If this is a top-level BrowsingContext, tell the frameloader it's time