Closed
Bug 1309900
Opened 8 years ago
Closed 8 years ago
Subframe history navigation logic gives false positives.
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | verified |
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(2 files)
The logic at [1] can give false positives when you have two top level history entries with children. In non-e10s that doesn't matter much because the docshell IDs still match and you end up hitting [2], so things happen to work. Not sure if the docshell IDs could ever be different, in which case it would break. However, if the two history entries are in separate processes, the docshell IDs don't match and you hit [3]. This means either you go back too far or it fails completely. STR: 1) navigate to about:about 2) navigate to about:accounts (which uses iframes) 3) navigate to data:text/html,<iframe src="about:blank"></iframe> 4) go back You end up at about:about not about:accounts. If you try to go forward twice you stop at about:accounts, because there's nothing past the data: URI. If you use the drop down to jump from about:about to the data: URI it works because about:about doesn't have children. [1] https://hg.mozilla.org/mozilla-central/file/3f37edc0d6e1/docshell/shistory/nsSHistory.cpp#l1601 [2] https://hg.mozilla.org/mozilla-central/file/3f37edc0d6e1/docshell/shistory/nsSHistory.cpp#l1664 [3] https://hg.mozilla.org/mozilla-central/file/3f37edc0d6e1/docshell/shistory/nsSHistory.cpp#l1611
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=371a44fbbca5
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Sorry for the separate neddinfo as well, but I've struggled to create a test for the STR in comment 0, because of the process switch for the browser involved. Do you have any advice on how I could go about testing it? I don't know of any other circumstance that exposes the underlying bug.
Flags: needinfo?(bugs)
Comment 4•8 years ago
|
||
Oh, if this is only about process switch, and that is known to be buggy and will be replace with a real cross process session history. But, you should be able write a browser chrome test for this.
Flags: needinfo?(bugs)
Comment 6•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #0) > The logic at [1] can give false positives when you have two top level > history entries with children. > > In non-e10s that doesn't matter much because the docshell IDs still match > and you end up hitting [2], so things happen to work. But [2] isn't about docshell IDs, but SHEntry IDs. As far as I see, DocShell IDs may or may not match, and same with SHEntry IDs. Or, perhaps something in session store is trying to handle different processes? It should be possible to have different docshell ids or shentry ids also in non-e10s. Just have different pages with iframes. Only their top level page share the same ID.
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8801806 [details] Bug 1309900: Fix subframe history navigation logic. https://reviewboard.mozilla.org/r/86448/#review85296 I think I need to see the testcase too before reviewing, and I don't quite understand the explanation in the bug. It talks about docshell IDs, but links to code which is about SHEntry IDs. Could you explain a bit more. ::: docshell/shistory/nsSHistory.cpp:1609 (Diff revision 1) > - return InitiateLoad(nextEntry, docShell, aLoadType); > } > > nsresult > -nsSHistory::CompareFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, > - nsIDocShell* aParent, long aLoadType, > +nsSHistory::LoadDifferingFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, > + nsIDocShell* aDocShell, long aLoadType, What aDocShell? IMO aParent makes more sense
Attachment #8801806 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6) > (In reply to Bob Owen (:bobowen) from comment #0) > > The logic at [1] can give false positives when you have two top level > > history entries with children. > > > > In non-e10s that doesn't matter much because the docshell IDs still match > > and you end up hitting [2], so things happen to work. > But [2] isn't about docshell IDs, but SHEntry IDs. I meant it gets past the docshell ID check above this and makes it to this line. As the two SHEntry IDs don't match it does the load ... a top level load in this case. In the cross process case the docshell ID check fails and we end up going to the next history entry. > It should be possible to have different docshell ids or shentry ids also in > non-e10s. > Just have different pages with iframes. Only their top level page share the > same ID. This problem is only for the top level aPrevEntry and aNextEntry, if the docshell IDs can be different for these (when same process) and they both have child frames, then that would fail in release as well. Because of code further down, the docshell IDs are guaranteed to match in the recursive calls. (In reply to Olli Pettay [:smaug] from comment #7) > I think I need to see the testcase too before reviewing, and I don't quite > understand the explanation in the bug. OK, I'll try again to get it working. > > -nsSHistory::CompareFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, > > - nsIDocShell* aParent, long aLoadType, > > +nsSHistory::LoadDifferingFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, > > + nsIDocShell* aDocShell, long aLoadType, > > What aDocShell? IMO aParent makes more sense Because the SHEntry have SHEntry parents as well, I though aParent was a bit confusing.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8801806 [details] Bug 1309900: Fix subframe history navigation logic. https://reviewboard.mozilla.org/r/86448/#review85296 > What aDocShell? IMO aParent makes more sense I've changed this back, although as I explained in comment 8 I still think it's a bit confusing.
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f53fe3b7a82667a1573d75b5f4f3fdcbaa12247
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8801806 [details] Bug 1309900: Fix subframe history navigation logic. https://reviewboard.mozilla.org/r/86448/#review86002 This is session history, so regressions are very likely. Just be prepared ;) ::: docshell/shistory/nsSHistory.h:56 (Diff revision 2) > friend class nsSHEnumerator; > friend class nsSHistoryObserver; > > // Could become part of nsIWebNavigation > NS_IMETHOD GetTransactionAtIndex(int32_t aIndex, nsISHTransaction** aResult); > - nsresult CompareFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, > + nsresult LoadDifferingFrames(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, Per IRC, call this LoadDifferingEntries
Attachment #8801806 -
Flags: review?(bugs) → review+
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8802544 [details] Bug 1309900 Test: Ensure cross process history works when both history entries have children. https://reviewboard.mozilla.org/r/86928/#review86016
Attachment #8802544 -
Flags: review?(bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/faec341f8952 Fix subframe history navigation logic. r=smaug https://hg.mozilla.org/integration/autoland/rev/b3954dee3eda Test: Ensure cross process history works when both history entries have children. r=smaug
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/faec341f8952 https://hg.mozilla.org/mozilla-central/rev/b3954dee3eda
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 22•8 years ago
|
||
Reproduced this issue using the Nightly 52.0a1 (Build ID: 20161013030204) on Windows 10 x64. Verified as fixed using Firefox 52 beta 9 (Build ID: 20170223185858) on Windows 10 x64, Ubuntu 16.04 x 64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•