Last Comment Bug 685782 - nsDocShell::SetHistoryEntry doesn't sync correctly when |this| has a parent that is itself a subframe
: nsDocShell::SetHistoryEntry doesn't sync correctly when |this| has a parent t...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 668728
  Show dependency treegraph
 
Reported: 2011-09-08 21:30 PDT by :Ehsan Akhgari (out sick)
Modified: 2012-01-06 06:41 PST (History)
5 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (17.48 KB, patch)
2011-09-08 21:31 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Actually simpler testcase that shows an issue with history navigation after reload with iframes (169 bytes, text/html)
2011-09-23 20:00 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
SetHistoryEntry should start syncing at the root of the docshell tree, not at its parent docshell. (1.66 KB, patch)
2011-09-23 20:05 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
Details | Diff | Review

Description :Ehsan Akhgari (out sick) 2011-09-08 21:30:37 PDT
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 :Ehsan Akhgari (out sick) 2011-09-08 21:31:36 PDT
Created attachment 559382 [details] [diff] [review]
Patch (v1)
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-09 14:23:28 PDT
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 :Ehsan Akhgari (out sick) 2011-09-09 14:26:36 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-09 14:32:09 PDT
> 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 :Ehsan Akhgari (out sick) 2011-09-09 15:56:07 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-09 19:03:57 PDT
> 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 :Ehsan Akhgari (out sick) 2011-09-10 17:47:21 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-10 19:26:30 PDT
I'll take a look at it on Monday.  I meant to do it Friday, but ran out of time...
Comment 9 :Ehsan Akhgari (out sick) 2011-09-12 07:25:03 PDT
Sounds great, thanks :)
Comment 10 :Ms2ger 2011-09-18 12:01:23 PDT
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 :Ehsan Akhgari (out sick) 2011-09-21 15:01:48 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-21 19:02:50 PDT
Not yet.  It's on my short list....
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-23 19:28:17 PDT
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 [:bz] (Out June 25-July 6) 2011-09-23 19:58:28 PDT
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 [:bz] (Out June 25-July 6) 2011-09-23 20:00:53 PDT
Created attachment 562207 [details]
Actually simpler testcase that shows an issue with history navigation after reload with iframes
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-23 20:04:10 PDT
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 [:bz] (Out June 25-July 6) 2011-09-23 20:05:44 PDT
Created attachment 562208 [details] [diff] [review]
SetHistoryEntry should start syncing at the root of the docshell tree, not at its parent docshell.
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-30 08:02:46 PDT
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 [:bz] (Out June 25-July 6) 2011-10-03 12:14:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f7a86b4cb1
Comment 20 Matt Brubeck (:mbrubeck) 2011-10-03 16:51:56 PDT
https://hg.mozilla.org/mozilla-central/rev/69f7a86b4cb1

Note You need to log in before you can comment on or make changes to this bug.