Bug 1729662 - UNED exam page flickers / reloads infinitely. r=smaug

When we navigate in history to the same entry that we're current at then we
actually do a reload. The problem is in the way we detect whether to do a reload
in the parent process.

If a page does a back and a forward one after the other in a script, then the
parent will calculate the index for the back and tell the child to load the
entry at that index. While the child is processing the load of that entry, the
BC in the parent process still has the same entry as its active entry (until the
child commits the load of the entry over IPC). The parent then processes the
forward, calculates the index for the forward and finds the entry at that index.
This is the same entry that we were at before doing anything, and so the same
entry as the active entry in the BC in the parent process. We used to compare
the entry that we're going to load with the active entry in the BC to determine
whether we're doing a reload, and so in this situation we would assume the
forward navigation was actually doing a reload. The child would reload the page,
and we'd run the script again and we'd end up in a reload loop.

Comparing the offset with 0 to determine whether we're doing a reload fixes this
issue.

Differential Revision: https://phabricator.services.mozilla.com/D126585
This commit is contained in:
Peter Van der Beken 2021-10-04 15:01:30 +00:00
parent 939934c85c
commit 0cb68caed4
15 changed files with 178 additions and 58 deletions

View File

@ -1095,7 +1095,7 @@ void CanonicalBrowsingContext::HistoryGo(
// GoToIndex checks that index is >= 0 and < length.
nsTArray<nsSHistory::LoadEntryResult> loadResults;
nsresult rv = shistory->GotoIndex(index.value(), loadResults, sameEpoch,
aUserActivation);
aOffset == 0, aUserActivation);
if (NS_FAILED(rv)) {
MOZ_LOG(gSHLog, LogLevel::Debug,
("Dropping HistoryGo - bad index or same epoch (not in same doc)"));

View File

@ -8635,7 +8635,7 @@ bool nsDocShell::IsSameDocumentNavigation(nsDocShellLoadState* aLoadState,
}
if (aState.mHistoryNavBetweenSameDoc &&
!aLoadState->GetLoadingSessionHistoryInfo()->mLoadingCurrentActiveEntry) {
!aLoadState->GetLoadingSessionHistoryInfo()->mLoadingCurrentEntry) {
return true;
}
@ -11750,13 +11750,12 @@ nsresult nsDocShell::LoadHistoryEntry(const LoadingSessionHistoryInfo& aEntry,
loadState->SetHasValidUserGestureActivation(
loadState->HasValidUserGestureActivation() || aUserActivation);
return LoadHistoryEntry(loadState, aLoadType,
aEntry.mLoadingCurrentActiveEntry);
return LoadHistoryEntry(loadState, aLoadType, aEntry.mLoadingCurrentEntry);
}
nsresult nsDocShell::LoadHistoryEntry(nsDocShellLoadState* aLoadState,
uint32_t aLoadType,
bool aReloadingActiveEntry) {
bool aLoadingCurrentEntry) {
if (!IsNavigationAllowed()) {
return NS_OK;
}
@ -11777,7 +11776,7 @@ nsresult nsDocShell::LoadHistoryEntry(nsDocShellLoadState* aLoadState,
rv = CreateAboutBlankContentViewer(
aLoadState->PrincipalToInherit(),
aLoadState->PartitionedPrincipalToInherit(), nullptr, nullptr,
/* aIsInitialDocument */ false, Nothing(), !aReloadingActiveEntry);
/* aIsInitialDocument */ false, Nothing(), !aLoadingCurrentEntry);
if (NS_FAILED(rv)) {
// The creation of the intermittent about:blank content

View File

@ -987,7 +987,7 @@ class nsDocShell final : public nsDocLoader,
const mozilla::dom::LoadingSessionHistoryInfo& aEntry, uint32_t aLoadType,
bool aUserActivation);
nsresult LoadHistoryEntry(nsDocShellLoadState* aLoadState, uint32_t aLoadType,
bool aReloadingActiveEntry);
bool aLoadingCurrentEntry);
nsresult GetHttpChannel(nsIChannel* aChannel, nsIHttpChannel** aReturn);
nsresult ConfirmRepost(bool* aRepost);
nsresult GetPromptAndStringBundle(nsIPrompt** aPrompt,

View File

@ -556,12 +556,11 @@ nsDocShellLoadState::GetLoadingSessionHistoryInfo() const {
}
void nsDocShellLoadState::SetLoadIsFromSessionHistory(
int32_t aOffset, bool aLoadingFromActiveEntry) {
int32_t aOffset, bool aLoadingCurrentEntry) {
if (mLoadingSessionHistoryInfo) {
mLoadingSessionHistoryInfo->mLoadIsFromSessionHistory = true;
mLoadingSessionHistoryInfo->mOffset = aOffset;
mLoadingSessionHistoryInfo->mLoadingCurrentActiveEntry =
aLoadingFromActiveEntry;
mLoadingSessionHistoryInfo->mLoadingCurrentEntry = aLoadingCurrentEntry;
}
}

View File

@ -326,8 +326,7 @@ class nsDocShellLoadState final {
mozilla::dom::DocShellLoadStateInit Serialize();
void SetLoadIsFromSessionHistory(int32_t aOffset,
bool aLoadingFromActiveEntry);
void SetLoadIsFromSessionHistory(int32_t aOffset, bool aLoadingCurrentEntry);
void ClearLoadIsFromSessionHistory();
void MaybeStripTrackerQueryStrings(mozilla::dom::BrowsingContext* aContext,

View File

@ -350,7 +350,7 @@ LoadingSessionHistoryInfo::LoadingSessionHistoryInfo(
mLoadId(aInfo->mLoadId),
mLoadIsFromSessionHistory(aInfo->mLoadIsFromSessionHistory),
mOffset(aInfo->mOffset),
mLoadingCurrentActiveEntry(aInfo->mLoadingCurrentActiveEntry) {
mLoadingCurrentEntry(aInfo->mLoadingCurrentEntry) {
MOZ_ASSERT(SessionHistoryEntry::sLoadIdToEntry &&
SessionHistoryEntry::sLoadIdToEntry->Get(mLoadId) == aEntry);
}
@ -1586,7 +1586,7 @@ void IPDLParamTraits<dom::LoadingSessionHistoryInfo>::Write(
WriteIPDLParam(aMsg, aActor, aParam.mLoadId);
WriteIPDLParam(aMsg, aActor, aParam.mLoadIsFromSessionHistory);
WriteIPDLParam(aMsg, aActor, aParam.mOffset);
WriteIPDLParam(aMsg, aActor, aParam.mLoadingCurrentActiveEntry);
WriteIPDLParam(aMsg, aActor, aParam.mLoadingCurrentEntry);
WriteIPDLParam(aMsg, aActor, aParam.mForceMaybeResetName);
}
@ -1598,8 +1598,7 @@ bool IPDLParamTraits<dom::LoadingSessionHistoryInfo>::Read(
!ReadIPDLParam(aMsg, aIter, aActor,
&aResult->mLoadIsFromSessionHistory) ||
!ReadIPDLParam(aMsg, aIter, aActor, &aResult->mOffset) ||
!ReadIPDLParam(aMsg, aIter, aActor,
&aResult->mLoadingCurrentActiveEntry) ||
!ReadIPDLParam(aMsg, aIter, aActor, &aResult->mLoadingCurrentEntry) ||
!ReadIPDLParam(aMsg, aIter, aActor, &aResult->mForceMaybeResetName)) {
aActor->FatalError("Error reading fields for LoadingSessionHistoryInfo");
return false;

View File

@ -230,12 +230,12 @@ struct LoadingSessionHistoryInfo {
// but session-history-in-parent needs to pass needed information explicitly
// to the relevant child process.
bool mLoadIsFromSessionHistory = false;
// mOffset and mLoadingCurrentActiveEntry are relevant only if
// mOffset and mLoadingCurrentEntry are relevant only if
// mLoadIsFromSessionHistory is true.
int32_t mOffset = 0;
// If we're loading from the current active entry we want to treat it as not
// a same-document navigation (see nsDocShell::IsSameDocumentNavigation).
bool mLoadingCurrentActiveEntry = false;
// If we're loading from the current entry we want to treat it as not a
// same-document navigation (see nsDocShell::IsSameDocumentNavigation).
bool mLoadingCurrentEntry = false;
// If mForceMaybeResetName.isSome() is true then the parent process has
// determined whether the BC's name should be cleared and stored in session
// history (see https://html.spec.whatwg.org/#history-traversal step 4.2).

View File

@ -1404,7 +1404,8 @@ nsresult nsSHistory::Reload(uint32_t aReloadFlags,
}
nsresult rv = LoadEntry(
mIndex, loadType, HIST_CMD_RELOAD, aLoadResults, /*aSameEpoch*/ false,
mIndex, loadType, HIST_CMD_RELOAD, aLoadResults, /* aSameEpoch */ false,
/* aLoadCurrentEntry */ true,
aReloadFlags & nsIWebNavigation::LOAD_FLAGS_USER_ACTIVATION);
if (NS_FAILED(rv)) {
aLoadResults.Clear();
@ -1429,7 +1430,9 @@ nsresult nsSHistory::ReloadCurrentEntry(
// Notify listeners
NOTIFY_LISTENERS(OnHistoryGotoIndex, ());
return LoadEntry(mIndex, LOAD_HISTORY, HIST_CMD_RELOAD, aLoadResults);
return LoadEntry(mIndex, LOAD_HISTORY, HIST_CMD_RELOAD, aLoadResults,
/* aSameEpoch */ false, /* aLoadCurrentEntry */ true,
/* aUserActivation */ false);
}
void nsSHistory::EvictOutOfRangeWindowContentViewers(int32_t aIndex) {
@ -1964,8 +1967,8 @@ nsSHistory::UpdateIndex() {
NS_IMETHODIMP
nsSHistory::GotoIndex(int32_t aIndex, bool aUserActivation) {
nsTArray<LoadEntryResult> loadResults;
nsresult rv =
GotoIndex(aIndex, loadResults, /*aSameEpoch*/ false, aUserActivation);
nsresult rv = GotoIndex(aIndex, loadResults, /*aSameEpoch*/ false,
aIndex == mIndex, aUserActivation);
NS_ENSURE_SUCCESS(rv, rv);
LoadURIs(loadResults);
@ -1982,9 +1985,10 @@ nsSHistory::EnsureCorrectEntryAtCurrIndex(nsISHEntry* aEntry) {
nsresult nsSHistory::GotoIndex(int32_t aIndex,
nsTArray<LoadEntryResult>& aLoadResults,
bool aSameEpoch, bool aUserActivation) {
bool aSameEpoch, bool aLoadCurrentEntry,
bool aUserActivation) {
return LoadEntry(aIndex, LOAD_HISTORY, HIST_CMD_GOTOINDEX, aLoadResults,
aSameEpoch, aUserActivation);
aSameEpoch, aLoadCurrentEntry, aUserActivation);
}
NS_IMETHODIMP_(bool)
@ -1999,15 +2003,16 @@ nsSHistory::HasUserInteractionAtIndex(int32_t aIndex) {
nsresult nsSHistory::LoadNextPossibleEntry(
int32_t aNewIndex, long aLoadType, uint32_t aHistCmd,
nsTArray<LoadEntryResult>& aLoadResults, bool aUserActivation) {
nsTArray<LoadEntryResult>& aLoadResults, bool aLoadCurrentEntry,
bool aUserActivation) {
mRequestedIndex = -1;
if (aNewIndex < mIndex) {
return LoadEntry(aNewIndex - 1, aLoadType, aHistCmd, aLoadResults,
/*aSameEpoch*/ false, aUserActivation);
/*aSameEpoch*/ false, aLoadCurrentEntry, aUserActivation);
}
if (aNewIndex > mIndex) {
return LoadEntry(aNewIndex + 1, aLoadType, aHistCmd, aLoadResults,
/*aSameEpoch*/ false, aUserActivation);
/*aSameEpoch*/ false, aLoadCurrentEntry, aUserActivation);
}
return NS_ERROR_FAILURE;
}
@ -2015,7 +2020,8 @@ nsresult nsSHistory::LoadNextPossibleEntry(
nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,
uint32_t aHistCmd,
nsTArray<LoadEntryResult>& aLoadResults,
bool aSameEpoch, bool aUserActivation) {
bool aSameEpoch, bool aLoadCurrentEntry,
bool aUserActivation) {
MOZ_LOG(gSHistoryLog, LogLevel::Debug,
("LoadEntry(%d, 0x%lx, %u)", aIndex, aLoadType, aHistCmd));
if (!mRootBC) {
@ -2089,15 +2095,15 @@ nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,
if (mRequestedIndex == mIndex) {
// Possibly a reload case
InitiateLoad(nextEntry, mRootBC, aLoadType, aLoadResults, aUserActivation,
requestedOffset);
InitiateLoad(nextEntry, mRootBC, aLoadType, aLoadResults, aLoadCurrentEntry,
aUserActivation, requestedOffset);
return NS_OK;
}
// Going back or forward.
bool differenceFound =
LoadDifferingEntries(prevEntry, nextEntry, mRootBC, aLoadType,
aLoadResults, aUserActivation, requestedOffset);
bool differenceFound = LoadDifferingEntries(
prevEntry, nextEntry, mRootBC, aLoadType, aLoadResults, aLoadCurrentEntry,
aUserActivation, requestedOffset);
if (!differenceFound) {
// LoadNextPossibleEntry will change the offset by one, and in order
// to keep track of the requestedOffset, need to reset mRequestedIndex to
@ -2105,7 +2111,7 @@ nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,
mRequestedIndex = originalRequestedIndex;
// We did not find any differences. Go further in the history.
return LoadNextPossibleEntry(aIndex, aLoadType, aHistCmd, aLoadResults,
aUserActivation);
aLoadCurrentEntry, aUserActivation);
}
return NS_OK;
@ -2115,6 +2121,7 @@ bool nsSHistory::LoadDifferingEntries(nsISHEntry* aPrevEntry,
nsISHEntry* aNextEntry,
BrowsingContext* aParent, long aLoadType,
nsTArray<LoadEntryResult>& aLoadResults,
bool aLoadCurrentEntry,
bool aUserActivation, int32_t aOffset) {
MOZ_ASSERT(aPrevEntry && aNextEntry && aParent);
@ -2125,8 +2132,8 @@ bool nsSHistory::LoadDifferingEntries(nsISHEntry* aPrevEntry,
if (prevID != nextID) {
// Set the Subframe flag if not navigating the root docshell.
aNextEntry->SetIsSubFrame(aParent != mRootBC);
InitiateLoad(aNextEntry, aParent, aLoadType, aLoadResults, aUserActivation,
aOffset);
InitiateLoad(aNextEntry, aParent, aLoadType, aLoadResults,
aLoadCurrentEntry, aUserActivation, aOffset);
return true;
}
@ -2185,7 +2192,7 @@ bool nsSHistory::LoadDifferingEntries(nsISHEntry* aPrevEntry,
// This will either load a new page to shell or some subshell or
// do nothing.
if (LoadDifferingEntries(pChild, nChild, bcChild, aLoadType, aLoadResults,
aUserActivation, aOffset)) {
aLoadCurrentEntry, aUserActivation, aOffset)) {
differenceFound = true;
}
}
@ -2195,7 +2202,8 @@ bool nsSHistory::LoadDifferingEntries(nsISHEntry* aPrevEntry,
void nsSHistory::InitiateLoad(nsISHEntry* aFrameEntry,
BrowsingContext* aFrameBC, long aLoadType,
nsTArray<LoadEntryResult>& aLoadResults,
bool aUserActivation, int32_t aOffset) {
bool aLoadCurrentEntry, bool aUserActivation,
int32_t aOffset) {
MOZ_ASSERT(aFrameBC && aFrameEntry);
LoadEntryResult* loadResult = aLoadResults.AppendElement();
@ -2216,20 +2224,18 @@ void nsSHistory::InitiateLoad(nsISHEntry* aFrameEntry,
loadState->SetSHEntry(aFrameEntry);
// If we're loading from the current active entry we want to treat it as not
// a same-document navigation (see nsDocShell::IsSameDocumentNavigation), so
// If we're loading the current entry we want to treat it as not a
// same-document navigation (see nsDocShell::IsSameDocumentNavigation), so
// record that here in the LoadingSessionHistoryEntry.
bool loadingFromActiveEntry;
bool loadingCurrentEntry;
if (mozilla::SessionHistoryInParent()) {
loadingFromActiveEntry =
aFrameBC->Canonical()->GetActiveSessionHistoryEntry() == aFrameEntry;
loadingCurrentEntry = aLoadCurrentEntry;
} else {
loadingFromActiveEntry =
loadingCurrentEntry =
aFrameBC->GetDocShell() &&
nsDocShell::Cast(aFrameBC->GetDocShell())->IsOSHE(aFrameEntry);
}
loadState->SetLoadIsFromSessionHistory(aOffset, loadingFromActiveEntry);
loadState->SetLoadIsFromSessionHistory(aOffset, loadingCurrentEntry);
if (mozilla::SessionHistoryInParent()) {
nsCOMPtr<SessionHistoryEntry> she = do_QueryInterface(aFrameEntry);

View File

@ -165,7 +165,8 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
nsTArray<LoadEntryResult>& aLoadResults);
nsresult ReloadCurrentEntry(nsTArray<LoadEntryResult>& aLoadResults);
nsresult GotoIndex(int32_t aIndex, nsTArray<LoadEntryResult>& aLoadResults,
bool aSameEpoch = false, bool aUserActivation = false);
bool aSameEpoch, bool aLoadCurrentEntry,
bool aUserActivation);
void WindowIndices(int32_t aIndex, int32_t* aOutStartIndex,
int32_t* aOutEndIndex);
@ -222,15 +223,17 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
mozilla::dom::BrowsingContext* aParent,
long aLoadType,
nsTArray<LoadEntryResult>& aLoadResults,
bool aUserActivation, int32_t aOffset);
bool aLoadCurrentEntry, bool aUserActivation,
int32_t aOffset);
void InitiateLoad(nsISHEntry* aFrameEntry,
mozilla::dom::BrowsingContext* aFrameBC, long aLoadType,
nsTArray<LoadEntryResult>& aLoadResult,
bool aUserActivation, int32_t aOffset);
bool aLoadCurrentEntry, bool aUserActivation,
int32_t aOffset);
nsresult LoadEntry(int32_t aIndex, long aLoadType, uint32_t aHistCmd,
nsTArray<LoadEntryResult>& aLoadResults,
bool aSameEpoch = false, bool aUserActivation = false);
nsTArray<LoadEntryResult>& aLoadResults, bool aSameEpoch,
bool aLoadCurrentEntry, bool aUserActivation);
// Find the history entry for a given bfcache entry. It only looks up between
// the range where alive viewers may exist (i.e nsSHistory::VIEWER_WINDOW).
@ -251,7 +254,7 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
nsresult LoadNextPossibleEntry(int32_t aNewIndex, long aLoadType,
uint32_t aHistCmd,
nsTArray<LoadEntryResult>& aLoadResults,
bool aUserActivation);
bool aLoadCurrentEntry, bool aUserActivation);
// aIndex is the index of the entry which may be removed.
// If aKeepNext is true, aIndex is compared to aIndex + 1,

View File

@ -0,0 +1,8 @@
<script>
addEventListener("load", () => {
(new BroadcastChannel("bug1729662")).postMessage("load");
history.pushState(1, null, location.href);
history.back();
history.forward();
});
</script>

View File

@ -109,6 +109,9 @@ support-files = file_bug675587.html
[test_bug1151421.html]
[test_bug1186774.html]
[test_bug1450164.html]
[test_bug1729662.html]
support-files =
file_bug1729662.html
[test_close_onpagehide_by_history_back.html]
[test_close_onpagehide_by_window_close.html]
[test_compressed_multipart.html]

View File

@ -0,0 +1,76 @@
<!DOCTYPE HTML>
<html>
<head>
<meta charset="utf-8">
<title>Test back/forward after pushState</title>
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
<script>
SimpleTest.waitForExplicitFinish();
SimpleTest.requestFlakyTimeout("Need to wait to make sure an event does not fire");
async function runTest() {
let win = window.open();
let goneBackAndForwardOnce = new Promise((resolve) => {
let timeoutID;
// We should only get one load event in win.
let bc = new BroadcastChannel("bug1729662");
bc.addEventListener("message", () => {
bc.addEventListener("message", () => {
clearTimeout(timeoutID);
resolve(false);
});
}, { once: true });
let goneBack = false, goneForward = false;
win.addEventListener("popstate", ({ state }) => {
// We should only go back and forward once, if we get another
// popstate after that then we should fall through to the
// failure case below.
if (!(goneBack && goneForward)) {
// Check if this is the popstate for the forward (the one for
// back will have state == undefined).
if (state == 1) {
ok(goneBack, "We should have gone back before going forward");
goneForward = true;
// Wait a bit to make sure there are no more popstate events.
// eslint-disable-next-line mozilla/no-arbitrary-setTimeout
timeoutID = setTimeout(resolve, 1000, true);
return;
}
// Check if we've gone back once before, if we get another
// popstate after that then we should fall through to the
// failure case below.
if (!goneBack) {
goneBack = true;
return;
}
}
clearTimeout(timeoutID);
resolve(false);
});
});
win.location = "file_bug1729662.html";
ok(await goneBackAndForwardOnce, "Stopped navigating history");
win.close();
SimpleTest.finish();
}
</script>
</head>
<body onload="runTest();">
<p id="display"></p>
<div id="content" style="display: none"></div>
<pre id="test"></pre>
</body>
</html>

View File

@ -1,3 +1,7 @@
[cross-document-traversal-cross-document-traversal.html]
expected:
if fission: ERROR
[cross-document traversals in the same (back) direction: the second is ignored]
expected:
if sessionHistoryInParent: FAIL
[cross-document traversals in the same (forward) direction: the second is ignored]
expected:
if sessionHistoryInParent: FAIL

View File

@ -1,3 +1,15 @@
[same-document-traversal-same-document-traversal-hashchange.html]
expected:
if fission: ERROR
if sessionHistoryInParent: TIMEOUT
[same-document traversals in opposite directions: queued up]
expected:
if sessionHistoryInParent: TIMEOUT
[same-document traversals in opposite directions, second traversal invalid at queuing time: queued up]
expected:
if sessionHistoryInParent: NOTRUN
[same-document traversals in the same (back) direction: queue up]
expected:
if sessionHistoryInParent: NOTRUN
[same-document traversals in the same (forward) direction: queue up]
expected:
if sessionHistoryInParent: NOTRUN

View File

@ -1,3 +1,15 @@
[same-document-traversal-same-document-traversal-pushstate.html]
expected:
if fission: ERROR
if sessionHistoryInParent: TIMEOUT
[same-document traversals in opposite directions: queued up]
expected:
if sessionHistoryInParent: TIMEOUT
[same-document traversals in opposite directions, second traversal invalid at queuing time: queued up]
expected:
if sessionHistoryInParent: NOTRUN
[same-document traversals in the same (back) direction: queue up]
expected:
if sessionHistoryInParent: NOTRUN
[same-document traversals in the same (forward) direction: queue up]
expected:
if sessionHistoryInParent: NOTRUN