From 1856a4dec3d6de9279152a2ad8ce8bd24d5992b9 Mon Sep 17 00:00:00 2001 From: Olli Pettay Date: Fri, 30 Oct 2020 22:10:15 +0000 Subject: [PATCH] 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 | 22 ++++++++++++++----- docshell/base/BrowsingContext.h | 3 ++- docshell/base/CanonicalBrowsingContext.cpp | 19 ++++++++-------- docshell/base/CanonicalBrowsingContext.h | 2 +- docshell/base/nsDocShell.cpp | 5 ++--- dom/ipc/ContentParent.cpp | 9 +++----- dom/ipc/ContentParent.h | 9 ++++---- dom/ipc/PContent.ipdl | 1 - ...nested-context-navigations-iframe.html.ini | 2 ++ 9 files changed, 39 insertions(+), 33 deletions(-) diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp index 2fd28d387034..72d24b700ea6 100644 --- a/docshell/base/BrowsingContext.cpp +++ b/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& 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()); } } diff --git a/docshell/base/BrowsingContext.h b/docshell/base/BrowsingContext.h index e8f08e87af26..b89f96af6610 100644 --- a/docshell/base/BrowsingContext.h +++ b/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& 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 diff --git a/docshell/base/CanonicalBrowsingContext.cpp b/docshell/base/CanonicalBrowsingContext.cpp index 73916db6ef13..35b919097980 100644 --- a/docshell/base/CanonicalBrowsingContext.cpp +++ b/docshell/base/CanonicalBrowsingContext.cpp @@ -516,18 +516,17 @@ void CanonicalBrowsingContext::SessionHistoryCommit(uint64_t aLoadId, mActiveEntry = newActiveEntry; } } else { - SessionHistoryEntry* parentEntry = - static_cast(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& 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. diff --git a/docshell/base/CanonicalBrowsingContext.h b/docshell/base/CanonicalBrowsingContext.h index f0aa6bc8843a..190ca6a68443 100644 --- a/docshell/base/CanonicalBrowsingContext.h +++ b/docshell/base/CanonicalBrowsingContext.h @@ -129,7 +129,7 @@ class CanonicalBrowsingContext final : public BrowsingContext { // See BrowsingContext::SetActiveSessionHistoryEntry. void SetActiveSessionHistoryEntry(const Maybe& aPreviousScrollPos, SessionHistoryInfo* aInfo, - uint32_t aLoadType, int32_t aChildOffset, + uint32_t aLoadType, uint32_t aUpdatedCacheKey, const nsID& aChangeID); diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index bec3f2699817..4b3524e05d6a 100644 --- a/docshell/base/nsDocShell.cpp +++ b/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? } diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index db85272d0993..2c8f69a257fb 100644 --- a/dom/ipc/ContentParent.cpp +++ b/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& aContext, const Maybe& 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(); } diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index 2e56a8a5b575..f588f3e2e581 100644 --- a/dom/ipc/ContentParent.h +++ b/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& aContext, const Maybe& 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& aContext, diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl index 8e7c2141e9c5..be6d73934558 100644 --- a/dom/ipc/PContent.ipdl +++ b/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); diff --git a/testing/web-platform/meta/resource-timing/nested-context-navigations-iframe.html.ini b/testing/web-platform/meta/resource-timing/nested-context-navigations-iframe.html.ini index 679ba23af0aa..60e33d02b081 100644 --- a/testing/web-platform/meta/resource-timing/nested-context-navigations-iframe.html.ini +++ b/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