Bug 1586911: Silence SHEntry assertion after local->remote->local iframe navigation. r=peterv
authorKris Maglione <maglione.k@gmail.com>
Fri, 11 Oct 2019 19:32:02 +0000
changeset 497301 c0029f42afd2873d7a7f3a5db4b40bfbb3865e18
parent 497300 b1a2422a6fa07aae5e95ebe3ac549548fb62785e
child 497302 aedc45de64b3e561e8f4ddd9311ac99af5367aff
push id36682
push userncsoregi@mozilla.com
push dateSat, 12 Oct 2019 09:52:03 +0000
treeherdermozilla-central@06ea2371f897 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1586911
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1586911: Silence SHEntry assertion after local->remote->local iframe navigation. r=peterv When we have a parser-created iframe which starts out in-process, transitions to remote, and then transitions back to in-process, we create separate DocShells for the first and last in-process loads. Since both are network-created, and have the same child index, they both try to add themselves as children to their parent's SHistory at the same index. And since the entry for the first DocShell already exists at that index when we try to add the second, that triggers an assertion. This isn't really ideal, but it is expected given the current state of session history under Fission. It should hopefully be solved more gracefully when the Fission-aware session history rewrite is done, but in the mean time, I think we should just ignore the conflict, since it's expected. Differential Revision: https://phabricator.services.mozilla.com/D48437
docshell/base/nsDocShell.cpp
docshell/shistory/nsISHEntry.idl
docshell/shistory/nsSHEntry.cpp
dom/file/tests/mochitest.ini
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -3455,17 +3455,17 @@ nsDocShell::AddChildSHEntry(nsISHEntry* 
      * hierarchy ie.,you just visited a frameset page
      */
     if (NS_FAILED(mLSHE->ReplaceChild(aNewEntry))) {
       rv = mLSHE->AddChild(aNewEntry, aChildOffset);
     }
   } else if (!aCloneRef) {
     /* This is an initial load in some subframe.  Just append it if we can */
     if (mOSHE) {
-      rv = mOSHE->AddChild(aNewEntry, aChildOffset);
+      rv = mOSHE->AddChild(aNewEntry, aChildOffset, UseRemoteSubframes());
     }
   } else {
     rv = AddChildSHEntryInternal(aCloneRef, aNewEntry, aChildOffset, aLoadType,
                                  aCloneChildren);
   }
   return rv;
 }
 
--- a/docshell/shistory/nsISHEntry.idl
+++ b/docshell/shistory/nsISHEntry.idl
@@ -378,17 +378,18 @@ interface nsISHEntry : nsISupports
      * This shared state is the SHEntry's BFCacheEntry.  So
      * hasBFCacheEntry(getSharedState()) is guaranteed to return true.
      */
     [noscript, notxpcom] nsSHEntryShared getSharedState();
 
     /**
      * Add a new child SHEntry. If offset is -1 adds to the end of the list.
      */
-    void AddChild(in nsISHEntry aChild, in long aOffset);
+    void AddChild(in nsISHEntry aChild, in long aOffset,
+                  [optional,default(false)] in bool aUseRemoteSubframes);
 
     /**
      * Remove a child SHEntry.
      */
     [noscript] void RemoveChild(in nsISHEntry aChild);
 
     /**
      * Get child at an index.
--- a/docshell/shistory/nsSHEntry.cpp
+++ b/docshell/shistory/nsSHEntry.cpp
@@ -582,17 +582,18 @@ nsSHEntry::GetLoadedInThisProcess(bool* 
 
 NS_IMETHODIMP
 nsSHEntry::GetChildCount(int32_t* aCount) {
   *aCount = mChildren.Count();
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsSHEntry::AddChild(nsISHEntry* aChild, int32_t aOffset) {
+nsSHEntry::AddChild(nsISHEntry* aChild, int32_t aOffset,
+                    bool aUseRemoteSubframes) {
   if (aChild) {
     NS_ENSURE_SUCCESS(aChild->SetParent(this), NS_ERROR_FAILURE);
   }
 
   if (aOffset < 0) {
     mChildren.AppendObject(aChild);
     return NS_OK;
   }
@@ -662,17 +663,26 @@ nsSHEntry::AddChild(nsISHEntry* aChild, 
         NS_ASSERTION(mChildren[aOffset + 1] == dynEntry, "Whaat?");
       }
     }
 
     // Make sure there isn't anything at aOffset.
     if (aOffset < mChildren.Count()) {
       nsISHEntry* oldChild = mChildren[aOffset];
       if (oldChild && oldChild != aChild) {
-        NS_ERROR(
+        // Under Fission, this can happen when a network-created iframe starts
+        // out in-process, moves out-of-process, and then switches back. At that
+        // point, we'll create a new network-created DocShell at the same index
+        // where we already have an entry for the original network-created
+        // DocShell.
+        //
+        // This should ideally stop being an issue once the Fission-aware
+        // session history rewrite is complete.
+        NS_ASSERTION(
+            aUseRemoteSubframes,
             "Adding a child where we already have a child? This may misbehave");
         oldChild->SetParent(nullptr);
       }
     }
 
     mChildren.ReplaceObjectAt(aChild, aOffset);
   }
 
--- a/dom/file/tests/mochitest.ini
+++ b/dom/file/tests/mochitest.ini
@@ -39,13 +39,13 @@ skip-if = (toolkit == 'android') # Andro
 skip-if = (toolkit == 'android') || (verify && !debug && (os == 'win')) # Android: Bug 775227
 [test_fileapi_slice_memFile_1.html]
 skip-if = (toolkit == 'android') # Android: Bug 775227
 [test_fileapi_slice_memFile_2.html]
 skip-if = (toolkit == 'android') # Android: Bug 775227
 [test_fileapi_slice_image.html]
 skip-if = (toolkit == 'android') # Android: Bug 775227
 [test_mozfiledataurl.html]
-skip-if = fission || toolkit == 'android' #TIMED_OUT
+skip-if = toolkit == 'android' #TIMED_OUT
 [test_bug1507893.html]
 support-files = worker_bug1507893.js
 [test_blob_reading.html]
 support-files = common_blob_reading.js worker_blob_reading.js