Closed
Bug 685782
Opened 13 years ago
Closed 13 years ago
nsDocShell::SetHistoryEntry doesn't sync correctly when |this| has a parent that is itself a subframe
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: ehsan.akhgari, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Updated•13 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
Assignee | ||
Comment 2•13 years ago
|
||
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•13 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.
Assignee | ||
Comment 4•13 years ago
|
||
> 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•13 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.)
Assignee | ||
Comment 6•13 years ago
|
||
> 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•13 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?
Assignee | ||
Comment 8•13 years ago
|
||
I'll take a look at it on Monday. I meant to do it Friday, but ran out of time...
Reporter | ||
Comment 9•13 years ago
|
||
Sounds great, thanks :)
Comment 10•13 years ago
|
||
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•13 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?
Assignee | ||
Comment 12•13 years ago
|
||
Not yet. It's on my short list....
Assignee | ||
Comment 13•13 years ago
|
||
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 | ||
Comment 14•13 years ago
|
||
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.
Assignee | ||
Comment 15•13 years ago
|
||
Assignee | ||
Comment 16•13 years ago
|
||
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. :(
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #562208 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [need review]
Comment 18•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [need review] → [need landing]
Assignee | ||
Comment 19•13 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla10
Comment 20•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•13 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.
Description
•