1609475 - Regression when navigating multiple subframes through history simultaneously

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Nika Layzell [:nika] (ni? for response)]

Repro Webpage: https://five-cartwheel.glitch.me/

STR:

1. Load a page with two iframes, both of which can be navigated using a link, such as https://five-cartwheel.glitch.me/.
2. Click on the link in iframe 1, followed by the link in iframe 2
3. Click and hold the "back" button, and select the history entry 2 entries ago.

Expected:
Both subframes navigate back, as-if the "back" button was pressed twice.

Actual:
Only the second frame navigates back, as-if the "back" button was pressed once.

Additional Notes:

• The currently active session history entry is the selected entry, however the state of the first iframe is out of sync.
• Pressing the "forward" button causes the first iframe to re-load

This was regressed at some point since Firefox 71 (the "stable" version I happen to have handy). I'm guessing it was regressed when we merged the session history + fission changes from ash into mozilla-central.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

I believe this is caused due to the LoadEntryResult outparameter from nsSHistory::LoadDifferingEntries and friends being a single entry to load, rather than a list of loads to perform. https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/docshell/shistory/nsSHistory.cpp#1475-1477

Rather than using a separate aDifferenceFound and aLoadResult, we should probably be using a nsTArray<LoadEntryResult> outparameter, and appending to it when differences are found.

Comment 2

[Peter Van der Beken [:peterv]]

Ugh. Our session history testcases are so incomplete :-(.

(In reply to :Nika Layzell (ni? for response) from comment #1)

Rather than using a separate aDifferenceFound and aLoadResult, we should probably be using a nsTArray<LoadEntryResult> outparameter, and appending to it when differences are found.

I think that would be the right fix, which is going to be more complicated with fission enabled of course.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

peterv, would you have time to look at this?
I think we should get this fixed before doing more the Fission changes, just make it easier to test everything.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1609475, history.go(>1) should possibly load several iframes, r=peterv

Tested locally with and without Fission and with and without the shistory-in-parent pref.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1609475, history.go(>1) should possibly load several iframes, r=annyG

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf4118d784f4936896f31c2b97b056ea8115e79

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1609475, history.go(>1) should possibly load several iframes, r=annyG

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bd2a22267d6
history.go(>1) should possibly load several iframes, r=annyG

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing test_sessionhistory.html

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=2bd2a22267d6585b9e2a420cd48ec23b7c91c475&selectedJob=289977227

Tier1 failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=289975211&repo=autoland&lineNumber=2799

Backout: https://hg.mozilla.org/integration/autoland/rev/e09bb8097c62c6e0f966a42e0a2c02a9ccadd1a8

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Oh, hmm, is it a silly mistake.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af35ae00a2e182c68a797f6056a07de3b6cda86

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c6a6c8deaf0
history.go(>1) should possibly load several iframes, r=annyG

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0c6a6c8deaf0

Comment 14

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1609475 - Expand on the comment about special behaviour in docshell/test/navigation/file_bug1609475.html, r=smaug

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bd301d8127f
Expand on the comment about special behaviour in docshell/test/navigation/file_bug1609475.html, r=smaug

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4bd301d8127f