685782 - nsDocShell::SetHistoryEntry doesn't sync correctly when |this| has a parent that is itself a subframe

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[(no longer active)]

Using window.history in mochitests in order to go backward/forward in history is poisonous, because it causes navigation in the TestRunner context.  I caught these three failures using my patch in bug 668728.  It turns out that these three tests were not really testing what they meant, as the history.back calls were causing the history in the context of TestRunner to go back.

I have a patch which opens the test files for those three bugs to open in their own window, so that the hisoty.back/forward calls would result in what these tests actually mean.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't quite understand the problem here... The first test there (test_bug413310.html) navigates a subframe of the test, then calls history.back().  This should navigate that subframe backwards... why is this a problem, exactly?  Is the problem that the history entries from that test stick around in the main test window's history and can affect later forward() calls, say?  Or something else?

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I don't quite understand the problem here... The first test there
> (test_bug413310.html) navigates a subframe of the test, then calls
> history.back().  This should navigate that subframe backwards... why is this
> a problem, exactly?  Is the problem that the history entries from that test
> stick around in the main test window's history and can affect later
> forward() calls, say?  Or something else?

history.back() is triggering navigation on the parent iframe, which is the one in which the test is running.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> history.back() is triggering navigation on the parent iframe

That's seriously broken.  Moving the test out to its own window won't fix that, will it?

Perhaps the right fix is to make the initial doNextStep call off a setTimeout from addLoadEvent.  If you do that, does the back() just navigate the subframe?

Comment 5

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> > history.back() is triggering navigation on the parent iframe
> 
> That's seriously broken.  Moving the test out to its own window won't fix
> that, will it?

It will, because the window that will be opened doesn't have a back history entry.  I verified that the fix works locally.

> Perhaps the right fix is to make the initial doNextStep call off a
> setTimeout from addLoadEvent.  If you do that, does the back() just navigate
> the subframe?

No.  Why would that make a difference?  (I tested it, and it doesn't.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Why would that make a difference?

Because it might make us create a separate shentry for the subframe.

I'd still like to understand why the parent page of the subframe is navigating.  That seems _really_ wrong.  Extremely so.  If it's happening, either I'm totally misreading the test or something is _very_ broken.

Comment 7

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > Why would that make a difference?
> 
> Because it might make us create a separate shentry for the subframe.
> 
> I'd still like to understand why the parent page of the subframe is
> navigating.  That seems _really_ wrong.  Extremely so.  If it's happening,
> either I'm totally misreading the test or something is _very_ broken.

I'm not really familiar with the session history code.  Where do I need to look in order to figure out what's going wrong here?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'll take a look at it on Monday.  I meant to do it Friday, but ran out of time...

Comment 9

[(no longer active)]

Sounds great, thanks :)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 559382 [details] [diff] [review]
Patch (v1)

>--- a/docshell/test/test_framedhistoryframes.html
>+++ b/docshell/test/test_framedhistoryframes.html

>+  gWindow = window.open("historyframes.html", "hisotryframes", "width=500,height=500");
                                                 ^^^^^^^^^^^^^

Intentional?

Comment 11

[(no longer active)]

(In reply to Ms2ger from comment #10)
> Comment on attachment 559382 [details] [diff] [review]
> Patch (v1)
> 
> >--- a/docshell/test/test_framedhistoryframes.html
> >+++ b/docshell/test/test_framedhistoryframes.html
> 
> >+  gWindow = window.open("historyframes.html", "hisotryframes", "width=500,height=500");
>                                                  ^^^^^^^^^^^^^
> 
> Intentional?

No!

Boris: did you manage to take a look at this?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Not yet.  It's on my short list....

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, so I finally got this sorted out.  There's actually a longstanding bug in docshell code that causes history trees to get out of sync.  It's sort of fine until someone does a reload, after which you lose.

Working on a targeted automated test for this, but the patch for that bug makes docshell tests pass for me when run with the patch for bug 668728.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hrm.  I can't seem to write a sane test for this.... the one test I wrote so far (which I will attach) fails even with my patch, even though I thought it represented the problem correctly.  Nevertheless, my patch does fix mochitests and is clearly _required_.  It may not be enough.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually simpler testcase that shows an issue with history navigation after reload with iframes

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, I can't figure out why my patch doesn't fix that testcase, so we probably need a separate bug on this.

I'm going to treat the fail that bug 668728 causes without the upcoming patch as a testcase for this bug, I guess.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: SetHistoryEntry should start syncing at the root of the docshell tree, not at its parent docshell.

Comment 18

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

Comment on attachment 562208 [details] [diff] [review]
SetHistoryEntry should start syncing at the root of the docshell tree, not at its parent docshell.

I thought I had reviewed this already last week.

Comment 19

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f7a86b4cb1

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/69f7a86b4cb1