426646 - Using location.replace breaks iframe history

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Rob]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

When using 2 dynamically generated iframes inserted into the page via Javascript innerHTML property, both loaded with about:blank at first and then populated using frames[framename].location.replace(url), Firefox appears to confuse the history items between the iframes.

Reproducible: Always

Steps to Reproduce:
1. Visit linked URL.
2. Click link in iframe1.
3. Click back button.
Actual Results:  
iframe2 moves to iframe1's previous URL in the history.

Expected Results:  
iframe1 should move to previous URL in its history.

Regression started at Firefox 3 Beta 4.  Beta 3 does not exhibit this bug.

Comment 1

[Martijn Wargers (dead)]

This regressed between 2008-02-26 and 2008-02-27:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-26+04&maxdate=2008-02-27+09&cvsroot=%2Fcvsroot

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Ok, this is a testcase based on that one from the url.
The 2nd frame should stay blank, and the 1st frame should say "1st page".
That doesn't happen in current trunk build. It says "2nd page" and "1st page" for the first and second frame.

Comment 3

[Damon Sicore (:damons)]

+'ing, as we need to find out exactly what's going on here.  We have a good regression range and a test case.

Comment 4

[Peter van der Woude [:Peter6]]

regressionwindow:
works in 20080226_0557_firefox-3.0b4pre.en-US.win32
fails in 20080226_0746_firefox-3.0b4pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1204034220&maxdate=1204040759&cvsroot=%2Fcvsroot

-> Bug 402982 ?
someone else will have to set the dependency ([sg])

Comment 5

[Martijn Wargers (dead)]

Peter, thanks for looking!

Comment 6

[Mike Schroepfer]

Assigning to smaug since his patch looks like the regression...

Comment 7

[Peter van der Woude [:Peter6]]

I wonder if this bug is the same as the intermittent breakage of history in Gmail.

repro:
open Gmail
open a mail in your inbox
press the back button on the navigation toolbar

result:
sometimes you go back to the inbox and sometimes the back/forward buttons turn both green w/o a different content on the screen. When you press back again Gmail totally reloads.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: WIP

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: WIP + mochitest

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 316011 [details] [diff] [review]
WIP + mochitest

Can you think something this would break.
I tried to look at docshell history to find some problematic cases, but haven't found any yet.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm...  So this will null the parent pointer on mDocShell, right?  Are we guaranteed that no script will run in it after that?  We use that parent pointer in security checks.  That's the first issue that comes to mind.  I have no real idea how this affects session history yet, but it should just be restoring the behavior we used to have in terms of when the docshell is removed from the tree, right?

I really wish we had some actual regression tests for session history.  Dealing with this stuff is like a minefield.  :(

Is it at all possible to get several people to work on that for a week or two at some point?

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #11)
> Hmm...  So this will null the parent pointer on mDocShell, right?  Are we
> guaranteed that no script will run in it after that?  We use that parent
> pointer in security checks.
Hmm, then the patch isn't good. Unload does fire after.

The problem the testcase shows is that it creates 3 iframe elements;
first one, which gets replaced by two new ones.
Replacement + creating 2 iframes happen within one document update.

/me continues debugging...

Comment 13

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #11)
> We use that parent pointer in security checks.
In which security checks?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> In which security checks?

nsScriptSecurityManager::CanExecuteScripts if nothing else.  There may be others; I don't recall where we use the parent document chain and where we use the docshell chain.

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: testcase for 1.8

1.8 branch has the same bug, but needs a bit different testcase.

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Just FYI, I've been debugging this some more, but since it means learning
all the SHEntry and Docshell's hierarchy code, it has taken some time.

Anyway, the problem is that parent SHEntry::mChildren.Count() == 3 (because 
entries are created using nsDocShell::mChildOffset) but when go(-1) is used, 
parent docshell has only 2 children.
After trying few different ways to fix this, I think the best one
might be to rename mChildOffset to mChildNumber (meaning it is always increased
when a child is added to parent but not decreased when removing a child.)
Then change the way child docshells are itarated in SH code - they
don't have same indexes as SHEntries. SHEntry's index would be the same as
mChildNumber. I'll try that ASAP.

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: proposed patch

Add mChildID which is used then to indicate the right entry in session history.
mChildOffset is currently used to do that, but because mChildOffset isn't kept
intact, things go wrong.
Session history may contain 'holes' or impossible-to-use entries.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/shistory/src/nsSHEntry.cpp&rev=1.62&mark=621-622#616
The patch tries to ensure that only entries related to existing docshells are
used.
The problem which still stays with the patch is that if some page
creates lots of <iframe>s and the removes those and creates new ones etc. 
session history gets large. But hopefully that is so edge case that we
don't need to care about that now.

Changes to nsWebBrowser.cpp are there just because it implements nsIDocShellTreeItem

Mats, perhaps you could look at this? You did some mChildOffset related fixes.

Comment 18

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 316601 [details] [diff] [review]
proposed patch

Boris, also you might be familiar with SHEntry and DocShell handling.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to think about this in enough depth to be able to really review a change like this until at least after May 5.

At first glance, how does this patch handle innerHTML sets on an ancestor of an iframe, with history navigation in the iframe before/after the innerHTML set.  How do we handle those now?  How does IE handle them?

Comment 20

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

The patch 'fixes' this http://mozilla.pettay.fi/moztests/iframe_history.html
gecko1.8 and 1.9(with the patch) goes back to the pages loaded to the first 
iframe.
IE7 doesn't go back to the first iframe, only to the pages related to 
the 2nd Iframe.
Safari doesn't allow *any* back functionality.
With Opera one can go back within the 2nd iframe, then few 'back' clicks
which don't do *anything*, and then to the previous page.

http://mozilla.pettay.fi/moztests/iframe_history_go.html
uses history.go(-2). With the patch we behave like 1.8 and Opera and IE7.
Safari doesn't seem to do anything with history.go(-2), like gecko1.9 without
the patch.

http://mozilla.pettay.fi/moztests/iframe_history_go_5.html uses .go(-5)
There the patch doesn't change our behavior - which means the first (replaced
using .innerHTML) iframe is sort-of restored.
Opera doesn't have that behavior, nor IE7. They go back to the previous page
if there is such.
Safari again doesn't do anything reasonable.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK.  So sounds like it basically gives us 1.8 compat on those testcases?

Comment 22

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Yes, in those testcases. Though the patch does fix attachment 316202 [details], which happens also on 1.8.

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: better, but riskier

This one might be better. Closer to the IE behavior - going back doesn't
go back to the removed iframe, but to the page which was loaded in the browser
before the frame owner page.
The transaction history still contains those entries so the drop-down of
back-forward button contains extra items. Opera has similar-kind of problem, 
it shows too few entries in the drop down.
For 1.9 attachment 316601 [details] [diff] [review] could be enough.

Mats, do you have time to review? Who else could do docshell/shentry reviews?

Comment 24

[Damon Sicore (:damons)]

Anyone other than boris who can SR?  He's sorta busy.  

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm not sure what to make of the first paragraph of comment 20 given that the before and after docshells have different ids.  Why do they get treated as "the same" by history?

Perhaps related, the indexing in nsSHistory::CompareFrames looks odd.  Starting at previousDocShellIndex means that for each 'i' we'll look at docshells located at both i-1 and i.  Not finding a docshell with id == i means that we'll end up with dsTreeItemChild set to the last child docshell no matter what.  Given the non-decreasing nature of ids, it's quite possible to have a situation where all ids in the list are at least as big as ncnt, and hence strictly greater than all possible values of 'i'.

Whatever that code is doing needs a nice clear explanation, preferably in a comment in the code.  That is, a comment explaining why it's doing what it's doing.

Comment 26

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #25)
> Not finding a docshell with id == i means that
> we'll end up with dsTreeItemChild set to the last child docshell no matter
> what.
Right, that is what the 'better patch' fixes, but it is hard to
figure out what 1.8 bugs we should emulate or should we just fix things.
(Unfortunately late time to fix SHistory)

Comment 27

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 317292 [details] [diff] [review]
better, but riskier

To fix not-in-bfcache cases mAddedChildrenCount should be cleared when loading something new to docshell, I think. Needs testing - will do tomorrow.

Comment 28

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: much simpler patch

This keeps the old (and not so good) mChildOffset. Just doesn't take account
child docshells which are in the finalization list. This should bring back
the 1.8 behavior, I think.
Obviously doesn't fix attachment 316202 [details].

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 317706 [details] [diff] [review]
much simpler patch

>Index: docshell/base/nsDocShell.cpp
>+            // the offset so that SH hierarchy matches more closely and more
>+            // likely the docshell hierarchy.

"more likely"?  Perhaps "so that the SH hierarchy is more likely to match the docshell hierarchy"?

Will putting the shentry at the offset where something already exists just silently replace whatever is there?  If so, that sounds perfect.  Please double-check this part.

>+              do_GetInterface(static_cast<nsIDocShell*>(this));

  do_GetInterface(GetAsSuports(this))

please.

>+            PRUint32 offset = mChildList.Count() - 1;

OK.  So |offset| is at least as big as the length of mChildList before we just added the docshell, right?

>+               PRUint32 childCount = mChildList.Count();

Can we skip calling Count() twice?  I guess it doesn't matter that much...

>+                 if (doc->WillFrameLoaderBeFinalized(child) && offset) {

So the only way I see for |offset| to be 0 here while |child| will be finalized is if all the existing kids _and_ the new one we just added are being finalized.  I'd rather address them by just not walking the kid we just added, at which point the offset can't possible go negative.

>Index: content/base/public/nsIDocument.h
>+  virtual PRBool WillFrameLoaderBeFinalized(nsIDocShell* aShell) = 0;

FrameLoaderScheduledToBeFinalized(), maybe?  Or better yet, since we're talking about an nsIDocShell, DocshellWillBeFinalized or DocshellIsZombie or some such?

>+++ content/base/test/test_bug426646.html	>+    open("file_bug426646-" + testNumber + ".html", "", "");

I think it would be clearer to make this window.open().  And please put "width=100,height=100" in the options, so that it plays nice with non-tab-new-window settings?

>+++ content/base/test/file_bug426646-1.html
>+  window.frames[0].location.replace(url1);
>+  setTimeout(doe2, 100);

This is assuming the load will complete faster than 100ms.  That's not a great assumption.  I'd use an onload attribute on the relevant iframe to drive things, so that we really do catch the load completion.  If we want doe2 to not happen right after the load completes, the onload should be setting a 0-length timeout.

Similar for the other setTimeouts.

Note that you probably want to add comments about any assumptions about bfcache that this test is making.

>+function doe4() {
>+  opener.ok(window.frames[0].location == url1,

Please use |is(x, y, reason)| in preference to |ok(x == y, reason)|.  The former reports more useful output.  In particular, it'll do the "is" and "should be" parts for you, so your reason could just be "History.go(-1) didn't work?"

Same for the other ok() caller.

>+++ content/base/test/file_bug426646-2.html	

Again, please use onload events instead of timeouts, and is() instead of ok().

I don't see tests for some of the other things we discussed and that the other proposed patches in this bug broke.  Specifically, at least:

  1) The load in subframe, then non-bfcache traversal
     of the parent docshell, then back twice test.
  2) The load page with subframe, do a frame traversal,
     do a non-bfcache traversal of the parent, go back,
     reload test.

Not sure whether I brought up other possible problems in that irc conversation...

I'm also not seeing tests for the other things that came up in this bug and that you mentioned testing in comment 20.  Those would be good to add too.

r=bzbarsky with the code nits fixed and the setTimeout orange-proneness removed.  If you have to land this before writing those other tests, I can understand, but please make sure they go in before we ship final, ok?

And if any of those happen to correspond to old shistory bugs (ask gavin?), it would be good to mark them in-testsuite+, so when we do a sweep through this component writing tests we don't duplicate the work...

Comment 30

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #29)
> Will putting the shentry at the offset where something already exists just
> silently replace whatever is there?  If so, that sounds perfect.  Please
> double-check this part.
Not really replace. When there is a new SHEntry tree ( == a new page has been 
loaded), the new entry is in the same place as the old one in the old SH tree.

> OK.  So |offset| is at least as big as the length of mChildList before we just
> added the docshell, right?
No. offset <= the index in the mChildList.

> FrameLoaderScheduledToBeFinalized(), maybe?  Or better yet, since we're talking
> about an nsIDocShell, DocshellWillBeFinalized or DocshellIsZombie or some such?
I prefer FrameLoaderScheduledToBeFinalized, since it is really the
FrameLoader which will be finalized.

Comment 31

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

> This is assuming the load will complete faster than 100ms.  That's not a great
> assumption.  I'd use an onload attribute on the relevant iframe to drive
> things, so that we really do catch the load completion.
The problem is that what is the "relevant" frame.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> No. offset <= the index in the mChildList.

Uh... how so?

>+            PRUint32 offset = mChildList.Count() - 1;

Since we've already added the child, offset == the index of the child at this point.  Then we want to subtract one for every zombie frameloader that comes before aChild.  At the end of the process offset may be 0, but it can't possibly become 0 until the last loop iteration.

> I prefer FrameLoaderScheduledToBeFinalized

I can live with that, sure.  It's a little weird to be passing in a docshell to a function called that, but not a big deal.  DocshellsFrameLoader... would be too long, for sure.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> The problem is that what is the "relevant" frame.

For the cases when you're doing loads, this is easy: the frame you're doing the load in.

For the history traversals, it's harder; can you put load events on both possible frames to handle that?  Or just put a load event on the "right" frame and then the test will just time out if the wrong frame is navigated?  That might be harder to debug as a test failure, I guess.

Comment 34

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #33)
> For the history traversals, it's harder; can you put load events on both
> possible frames to handle that?
"both". We have actually 3 iframes in the first testcase and
6 in the other one.
document.body.innerHTML = "<iframe src='about:blank'></iframe>";
document.body.innerHTML += "<iframe src='about:blank'></iframe>";
creates 3 iframes, of which the first one will be removed immediately.

Comment 35

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to comment #32)
> > No. offset <= the index in the mChildList.
> 
> Uh... how so?
I mean after the loop offset will be at most the last index in the mChildList

> Since we've already added the child, offset == the index of the child at this
> point.  Then we want to subtract one for every zombie frameloader that comes
> before aChild.  At the end of the process offset may be 0, but it can't
> possibly become 0 until the last loop iteration.
I'm changing this so that the last item in mChildList isn't iterated.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> I mean after the loop offset will be at most the last index in the mChildList

Sure.  My only concern was for the 0-check in the loop itself.

Comment 37

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: with nits fixed, tests using onload

I'll write some more tests asap.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Note that you probably want to add comments about any assumptions about
> bfcache that this test is making.

I thought about this some more, and it would be even better to explicitly assert those assumptions in the test, so if they start being violated the test will go orange...  I've had to do that in other docshell-related tests.  Basically, you set some properties on the window, perform navigation, perform history navigation.  If those properties you set are still there, bfcache happened; otherwise it didn't.  That sort of thing.  It's a mess, but...

Alternately, if you just need to make sure bfcache _doesn't_ happen, you could have the test twiddle the bfcache prefs when it starts/stops.

Ideally, of course, we'd have copies of all such tests for cases when bfcache does and doesn't happen.  Gotta love the multiple-codepath thing.  :(

Comment 39

[Damon Sicore (:damons)]

Comment on attachment 317902 [details] [diff] [review]
with nits fixed, tests using onload

a1.9+=damons

Comment 40

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Adding in‑testsuite? so that I remember to add more tests.