Bug 1671962 - Iframes loaded from restored session are loaded out of order when Fission enabled, r=peterv

Differential Revision: https://phabricator.services.mozilla.com/D94904

docshell/base/BrowsingContext.cpp

@@ -153,6 +153,17 @@ BrowsingContext* BrowsingContext::Top() {
   return bc;
 }
 
+int32_t BrowsingContext::IndexOf(BrowsingContext* aChild) {
+  int32_t index = -1;
+  for (BrowsingContext* child : Children()) {
+    ++index;
+    if (child == aChild) {
+      break;
+    }
+  }
+  return index;
+}
+
 WindowContext* BrowsingContext::GetTopWindowContext() {
   if (mParentWindow) {
     return mParentWindow->TopWindowContext();
@@ -2744,7 +2755,7 @@ bool BrowsingContext::IsPopupAllowed() {
 
 void BrowsingContext::SetActiveSessionHistoryEntry(
     const Maybe<nsPoint>& aPreviousScrollPos, SessionHistoryInfo* aInfo,
-    uint32_t aLoadType, int32_t aChildOffset, uint32_t aUpdatedCacheKey) {
+    uint32_t aLoadType, uint32_t aUpdatedCacheKey) {
   if (XRE_IsContentProcess()) {
     // XXX Why we update cache key only in content process case?
     if (aUpdatedCacheKey != 0) {
@@ -2757,12 +2768,11 @@ void BrowsingContext::SetActiveSessionHistoryEntry(
       changeID = shistory->AddPendingHistoryChange();
     }
     ContentChild::GetSingleton()->SendSetActiveSessionHistoryEntry(
-        this, aPreviousScrollPos, *aInfo, aLoadType, aChildOffset,
-        aUpdatedCacheKey, changeID);
+        this, aPreviousScrollPos, *aInfo, aLoadType, aUpdatedCacheKey,
+        changeID);
   } else {
-    Canonical()->SetActiveSessionHistoryEntry(aPreviousScrollPos, aInfo,
-                                              aLoadType, aChildOffset,
-                                              aUpdatedCacheKey, nsID());
+    Canonical()->SetActiveSessionHistoryEntry(
+        aPreviousScrollPos, aInfo, aLoadType, aUpdatedCacheKey, nsID());
   }
 }
 

docshell/base/BrowsingContext.h

@@ -369,6 +369,7 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
 
   BrowsingContext* GetParent() const;
   BrowsingContext* Top();
+  int32_t IndexOf(BrowsingContext* aChild);
 
   // NOTE: Unlike `GetEmbedderWindowGlobal`, `GetParentWindowContext` does not
   // cross toplevel content browser boundaries.
@@ -702,7 +703,7 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   // aUpdatedCacheKey is 0 then it will be ignored.
   void SetActiveSessionHistoryEntry(const Maybe<nsPoint>& aPreviousScrollPos,
                                     SessionHistoryInfo* aInfo,
-                                    uint32_t aLoadType, int32_t aChildOffset,
+                                    uint32_t aLoadType,
                                     uint32_t aUpdatedCacheKey);
 
   // Replace the active entry for this browsing context. This is used for

docshell/base/CanonicalBrowsingContext.cpp

@@ -516,18 +516,17 @@ void CanonicalBrowsingContext::SessionHistoryCommit(uint64_t aLoadId,
               mActiveEntry = newActiveEntry;
             }
           } else {
-            SessionHistoryEntry* parentEntry =
-                static_cast<CanonicalBrowsingContext*>(GetParent())
-                    ->mActiveEntry;
+            SessionHistoryEntry* parentEntry = GetParent()->mActiveEntry;
             // XXX What should happen if parent doesn't have mActiveEntry?
             //     Or can that even happen ever?
             if (parentEntry) {
               mActiveEntry = newActiveEntry;
-              // FIXME The docshell code sometime uses -1 for aChildOffset!
               // FIXME Using IsInProcess for aUseRemoteSubframes isn't quite
               //       right, but aUseRemoteSubframes should be going away.
-              parentEntry->AddChild(mActiveEntry, Children().Length() - 1,
-                                    IsInProcess());
+              parentEntry->AddChild(
+                  mActiveEntry,
+                  CreatedDynamically() ? -1 : GetParent()->IndexOf(this),
+                  IsInProcess());
             }
           }
         }
@@ -613,8 +612,7 @@ void CanonicalBrowsingContext::NotifyOnHistoryReload(
 
 void CanonicalBrowsingContext::SetActiveSessionHistoryEntry(
     const Maybe<nsPoint>& aPreviousScrollPos, SessionHistoryInfo* aInfo,
-    uint32_t aLoadType, int32_t aChildOffset, uint32_t aUpdatedCacheKey,
-    const nsID& aChangeID) {
+    uint32_t aLoadType, uint32_t aUpdatedCacheKey, const nsID& aChangeID) {
   nsISHistory* shistory = GetSessionHistory();
   if (!shistory) {
     return;
@@ -644,8 +642,9 @@ void CanonicalBrowsingContext::SetActiveSessionHistoryEntry(
       shistory->AddChildSHEntryHelper(oldActiveEntry, mActiveEntry, Top(),
                                       true);
     } else if (GetParent() && GetParent()->mActiveEntry) {
-      GetParent()->mActiveEntry->AddChild(mActiveEntry, aChildOffset,
-                                          UseRemoteSubframes());
+      GetParent()->mActiveEntry->AddChild(
+          mActiveEntry, CreatedDynamically() ? -1 : GetParent()->IndexOf(this),
+          UseRemoteSubframes());
     }
   }
   // FIXME Need to do the equivalent of EvictContentViewersOrReplaceEntry.

docshell/base/CanonicalBrowsingContext.h

@@ -129,7 +129,7 @@ class CanonicalBrowsingContext final : public BrowsingContext {
   // See BrowsingContext::SetActiveSessionHistoryEntry.
   void SetActiveSessionHistoryEntry(const Maybe<nsPoint>& aPreviousScrollPos,
                                     SessionHistoryInfo* aInfo,
-                                    uint32_t aLoadType, int32_t aChildOffset,
+                                    uint32_t aLoadType,
                                     uint32_t aUpdatedCacheKey,
                                     const nsID& aChangeID);
 

docshell/base/nsDocShell.cpp

@@ -8950,8 +8950,7 @@ nsresult nsDocShell::HandleSameDocumentNavigation(
         // FIXME We should probably just compute mChildOffset in the parent
         //       instead of passing it over IPC here.
         mBrowsingContext->SetActiveSessionHistoryEntry(
-            Some(scrollPos), mActiveEntry.get(), mLoadType, mChildOffset,
-            cacheKey);
+            Some(scrollPos), mActiveEntry.get(), mLoadType, cacheKey);
         // FIXME Do we need to update mPreviousEntryIndex and mLoadedEntryIndex?
       }
     }
@@ -11623,7 +11622,7 @@ void nsDocShell::UpdateActiveEntry(
     // FIXME We should probably just compute mChildOffset in the parent
     //       instead of passing it over IPC here.
     mBrowsingContext->SetActiveSessionHistoryEntry(
-        aPreviousScrollPos, mActiveEntry.get(), mLoadType, mChildOffset,
+        aPreviousScrollPos, mActiveEntry.get(), mLoadType,
         /* aCacheKey = */ 0);
     // FIXME Do we need to update mPreviousEntryIndex and mLoadedEntryIndex?
   }

dom/ipc/ContentParent.cpp

@@ -5416,8 +5416,7 @@ mozilla::ipc::IPCResult ContentParent::RecvSetupFamilyCharMap(
 }
 
 mozilla::ipc::IPCResult ContentParent::RecvGetHyphDict(
-    nsIURI* aURI, base::SharedMemoryHandle* aOutHandle,
-    uint32_t* aOutSize) {
+    nsIURI* aURI, base::SharedMemoryHandle* aOutHandle, uint32_t* aOutSize) {
   if (!aURI) {
     return IPC_FAIL_NO_REASON(this);
   }
@@ -7065,12 +7064,10 @@ ContentParent::RecvGetLoadingSessionHistoryInfoFromParent(
 mozilla::ipc::IPCResult ContentParent::RecvSetActiveSessionHistoryEntry(
     const MaybeDiscarded<BrowsingContext>& aContext,
     const Maybe<nsPoint>& aPreviousScrollPos, SessionHistoryInfo&& aInfo,
-    uint32_t aLoadType, int32_t aChildOffset, uint32_t aUpdatedCacheKey,
-    const nsID& aChangeID) {
+    uint32_t aLoadType, uint32_t aUpdatedCacheKey, const nsID& aChangeID) {
   if (!aContext.IsDiscarded()) {
     aContext.get_canonical()->SetActiveSessionHistoryEntry(
-        aPreviousScrollPos, &aInfo, aLoadType, aChildOffset, aUpdatedCacheKey,
-        aChangeID);
+        aPreviousScrollPos, &aInfo, aLoadType, aUpdatedCacheKey, aChangeID);
   }
   return IPC_OK();
 }

dom/ipc/ContentParent.h

@@ -1200,9 +1200,9 @@ class ContentParent final
       const uint32_t& aGeneration,
       const mozilla::fontlist::Pointer& aFamilyPtr);
 
-  mozilla::ipc::IPCResult RecvGetHyphDict(
-      nsIURI* aURIParams, base::SharedMemoryHandle* aOutHandle,
-      uint32_t* aOutSize);
+  mozilla::ipc::IPCResult RecvGetHyphDict(nsIURI* aURIParams,
+                                          base::SharedMemoryHandle* aOutHandle,
+                                          uint32_t* aOutSize);
 
   mozilla::ipc::IPCResult RecvNotifyBenchmarkResult(const nsString& aCodecName,
                                                     const uint32_t& aDecodeFPS);
@@ -1370,8 +1370,7 @@ class ContentParent final
   mozilla::ipc::IPCResult RecvSetActiveSessionHistoryEntry(
       const MaybeDiscarded<BrowsingContext>& aContext,
       const Maybe<nsPoint>& aPreviousScrollPos, SessionHistoryInfo&& aInfo,
-      uint32_t aLoadType, int32_t aChildOffset, uint32_t aUpdatedCacheKey,
-      const nsID& aChangeID);
+      uint32_t aLoadType, uint32_t aUpdatedCacheKey, const nsID& aChangeID);
 
   mozilla::ipc::IPCResult RecvReplaceActiveSessionHistoryEntry(
       const MaybeDiscarded<BrowsingContext>& aContext,

dom/ipc/PContent.ipdl

@@ -1690,7 +1690,6 @@ parent:
     async SetActiveSessionHistoryEntry(MaybeDiscardedBrowsingContext context,
                                        nsPoint? previousScrollPosition,
                                        SessionHistoryInfo info, uint32_t loadType,
-                                       int32_t childOffset,
                                        uint32_t updatedCacheKey, nsID changeID);
     async ReplaceActiveSessionHistoryEntry(
         MaybeDiscardedBrowsingContext context, SessionHistoryInfo info);

testing/web-platform/meta/resource-timing/nested-context-navigations-iframe.html.ini

@@ -1,4 +1,6 @@
 [nested-context-navigations-iframe.html]
+  max-asserts: 2 # SHIP gives some NS_ASSERTIONs which are being investigated.
+  min-asserts: 0
   [Test that iframe navigations are not observable by the parent, even after history navigations by the parent]
     disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1572932