Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r?smaug draft
authorSamael Wang <freesamael@gmail.com>
Thu, 24 Aug 2017 15:17:39 +0800
changeset 664558 d51c63c7974c22b7a70cbad98bf8e1cd22314a99
parent 660156 07a55e278cc59d43f841156dfffaa829cea14786
child 664559 3b3febea820cbf6360061d8bcd9c709f43dcf415
push id79723
push userbmo:sawang@mozilla.com
push dateThu, 14 Sep 2017 03:01:27 +0000
reviewerssmaug
bugs1364364
milestone57.0a1
Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r?smaug MozReview-Commit-ID: F8OTxbWIp5O
docshell/shistory/nsSHEntryShared.cpp
--- a/docshell/shistory/nsSHEntryShared.cpp
+++ b/docshell/shistory/nsSHEntryShared.cpp
@@ -45,17 +45,28 @@ nsSHEntryShared::nsSHEntryShared()
   , mSticky(true)
   , mDynamicallyCreated(false)
   , mExpired(false)
 {
 }
 
 nsSHEntryShared::~nsSHEntryShared()
 {
+  // The destruction can be caused by either the entry is removed from session
+  // history and no one holds the reference, or the whole session history is on
+  // destruction. We want to ensure that we invoke
+  // shistory->RemoveFromExpirationTracker for the former case.
   RemoveFromExpirationTracker();
+
+  // Calling RemoveDynEntriesForBFCacheEntry on destruction is unnecessary since
+  // there couldn't be any SHEntry holding this shared entry, and we noticed
+  // that calling RemoveDynEntriesForBFCacheEntry in the middle of
+  // nsSHistory::Release can cause a crash, so set mSHistory to null explicitly
+  // before RemoveFromBFCacheSync.
+  mSHistory = nullptr;
   if (mContentViewer) {
     RemoveFromBFCacheSync();
   }
 }
 
 NS_IMPL_ISUPPORTS(nsSHEntryShared, nsIBFCacheEntry, nsIMutationObserver)
 
 already_AddRefed<nsSHEntryShared>
@@ -163,76 +174,76 @@ nsSHEntryShared::SetContentViewer(nsICon
   return NS_OK;
 }
 
 nsresult
 nsSHEntryShared::RemoveFromBFCacheSync()
 {
   MOZ_ASSERT(mContentViewer && mDocument, "we're not in the bfcache!");
 
+  // The call to DropPresentationState could drop the last reference, so hold
+  // |this| until RemoveDynEntriesForBFCacheEntry finishes.
+  RefPtr<nsSHEntryShared> kungFuDeathGrip = this;
+
+  // DropPresentationState would clear mContentViewer.
   nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
   DropPresentationState();
 
-  // Warning! The call to DropPresentationState could have dropped the last
-  // reference to this object, so don't access members beyond here.
-
   if (viewer) {
     viewer->Destroy();
   }
 
+  // Now that we've dropped the viewer, we have to clear associated dynamic
+  // subframe entries.
+  nsCOMPtr<nsISHistoryInternal> shistory = do_QueryReferent(mSHistory);
+  if (shistory) {
+    shistory->RemoveDynEntriesForBFCacheEntry(this);
+  }
+
   return NS_OK;
 }
 
-class DestroyViewerEvent : public mozilla::Runnable
-{
-public:
-  DestroyViewerEvent(nsIContentViewer* aViewer, nsIDocument* aDocument)
-    : mozilla::Runnable("DestroyViewerEvent")
-    , mViewer(aViewer)
-    , mDocument(aDocument)
-  {
-  }
-
-  NS_IMETHOD Run() override
-  {
-    if (mViewer) {
-      mViewer->Destroy();
-    }
-    return NS_OK;
-  }
-
-  nsCOMPtr<nsIContentViewer> mViewer;
-  nsCOMPtr<nsIDocument> mDocument;
-};
-
 nsresult
 nsSHEntryShared::RemoveFromBFCacheAsync()
 {
   MOZ_ASSERT(mContentViewer && mDocument, "we're not in the bfcache!");
 
-  // Release the reference to the contentviewer asynchronously so that the
-  // document doesn't get nuked mid-mutation.
-
+  // Check it again to play safe in release builds.
   if (!mDocument) {
     return NS_ERROR_UNEXPECTED;
   }
-  nsCOMPtr<nsIRunnable> evt = new DestroyViewerEvent(mContentViewer, mDocument);
-  nsresult rv = mDocument->Dispatch(mozilla::TaskCategory::Other, evt.forget());
+
+  // DropPresentationState would clear mContentViewer & mDocument. Capture and
+  // release the references asynchronously so that the document doesn't get
+  // nuked mid-mutation.
+  nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
+  nsCOMPtr<nsIDocument> document = mDocument;
+  RefPtr<nsSHEntryShared> self = this;
+  nsresult rv = mDocument->Dispatch(mozilla::TaskCategory::Other,
+    NS_NewRunnableFunction("nsSHEntryShared::RemoveFromBFCacheAsync",
+    [self, viewer, document]() {
+      if (viewer) {
+        viewer->Destroy();
+      }
+
+      nsCOMPtr<nsISHistoryInternal> shistory = do_QueryReferent(self->mSHistory);
+      if (shistory) {
+        shistory->RemoveDynEntriesForBFCacheEntry(self);
+      }
+    }));
+
   if (NS_FAILED(rv)) {
-    NS_WARNING("failed to dispatch DestroyViewerEvent");
+    NS_WARNING("Failed to dispatch RemoveFromBFCacheAsync runnable.");
   } else {
     // Drop presentation. Only do this if we succeeded in posting the event
     // since otherwise the document could be torn down mid-mutation, causing
     // crashes.
     DropPresentationState();
   }
 
-  // Careful! The call to DropPresentationState could have dropped the last
-  // reference to this nsSHEntryShared, so don't access members beyond here.
-
   return NS_OK;
 }
 
 nsresult
 nsSHEntryShared::GetID(uint64_t* aID)
 {
   *aID = mID;
   return NS_OK;