The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: bz)

Tracking

Trunk
mozilla10
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 559382 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #559382 - Flags: review?(bzbarsky)

Updated

6 years ago
Summary: Three of the docshell tests use window.hisotry in a poisonous way → Three of the docshell tests use window.history in a poisonous way
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?
(Reporter)

Comment 3

6 years ago
(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.
> 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?
(Reporter)

Comment 5

6 years ago
(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.)
> 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.
(Reporter)

Comment 7

6 years ago
(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?
I'll take a look at it on Monday.  I meant to do it Friday, but ran out of time...
(Reporter)

Comment 9

6 years ago
Sounds great, thanks :)
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?
(Reporter)

Comment 11

6 years ago
(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?
Not yet.  It's on my short list....
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.
Assignee: ehsan → bzbarsky
No longer blocks: 413310, 602256, 668513
Summary: Three of the docshell tests use window.history in a poisonous way → nsDocShell::SetHistoryEntry doesn't sync correctly when |this| has a parent that is itself a subframe
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.
Created attachment 562207 [details]
Actually simpler testcase that shows an issue with history navigation after reload with iframes
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.  :(
Created attachment 562208 [details] [diff] [review]
SetHistoryEntry should start syncing at the root of the docshell tree, not at its parent docshell.
Attachment #562208 - Flags: review?(Olli.Pettay)
Whiteboard: [need review]
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.
Attachment #562208 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [need review] → [need landing]
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f7a86b4cb1
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/69f7a86b4cb1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Attachment #559382 - Attachment is obsolete: true
Attachment #559382 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.