Closed Bug 1309900 Opened 8 years ago Closed 8 years ago

Subframe history navigation logic gives false positives.

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(2 files)

Bug 1309900: Fix subframe history navigation logic.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1309900 Test: Ensure cross process history works when both history entries have children.
58 bytes, text/x-review-board-request
smaug
: review+
Details 
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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=371a44fbbca5
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)
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)
The code originates from bug 71756. Not too useful comments there.
(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 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-
(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 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.
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 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+
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
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
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
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: