Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug
authorSamael Wang <freesamael@gmail.com>
Thu, 24 Aug 2017 15:17:39 +0800
changeset 669684 1422e5ec0a70e348d88d9d7f9d7abb4915b5e76f
parent 669683 0177f0fcd0435686dee3f4354995de3c516be1c2
child 669685 cbc5a2fd926e9e8d5fb95e31fd9907f62766dc2f
push id81398
push userhikezoe@mozilla.com
push dateMon, 25 Sep 2017 08:14:31 +0000
reviewerssmaug
bugs1364364
milestone58.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;