301397 - Incorrect page displayed in iframe with bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Adrian Yee]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+

When navigating through pages inside an iframe, it displays the wrong page in
the iframe after visiting another page that isn't in the iframe.

Reproducible: Always

Steps to Reproduce:
1. open up a new window
2. go to any url
3. go to the test url (this was a simple test case from another bug I reported)
4. click on the link to go to the 2nd iframe page
5. go to any url
6. now if you press back 3 times to go back to the first page everything is fine
on each time you press back
7. however, if you go forward to the test page, you get the 2nd iframe page
instead of the 1st one (wrong).
8. go forward again and you get the 2nd iframe page (correct)
9. go forward, then back and you get the 1st iframe page instead of the 2nd (wrong).
Actual Results:  
In steps 7 and 9, the wrong pages gets displayed.

Expected Results:  
In step 7, the 1st iframe page should be shown.
In step 9, the 2nd iframe page should be shown.

Comment 1

[Andrew Schultz]

confirmed with seamonkey CVS build from today.  I hit this assertion when going
back to the page with the first iframe.

###!!! ASSERTION: User did not call nsIContentViewer::Destroy: '!mPresShell &&
!mPresContext', file
/build/andrew/moz-debug/mozilla/layout/base/nsDocumentViewer.cpp, line 526

Comment 2

[Brian Ryner (not reading)]

Sounds serious, targetting for beta.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch, part 1

I debugged this and found a few different things that worried me.  One of them
is that when the toplevel session history entry is cloned for the subframe
navigation, mOSHE is not updated on the toplevel docshell.  This causes the
content viewer to be cached on the wrong history entry when the next toplevel
navigation happens.  This by itself seems to be sufficient to fix the bug.

I don't really know this code very well, so it would be great to hear from
someone who does.

I uncovered a couple of other things that need to be fixed/investigated too:
- We need to make sure that we would never have a situation where a subframe,
and a parent content viewer that contains that frame, are both cached on
separate history entries.  This opens the door to destroying content viewers
prematurely or at unexpected times.
- Restoring a subframe from history doesn't seem to work... we decide that the
content viewer has the wrong container.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch, part 1

Boris reminded me that since we're cloning all of the entries (except for the
one we replace), we need to potentially reset mOSHE/mLSHE for the whole tree. 
This patch walks the docshell tree in parallel with the session history tree,
and updates mOSHE and/or mLSHE for cloned entries to point to the new entry. 
The walk involves a somewhat suboptimal search, since the session history tree
(a) may contain more children than the docshell tree, and (b) may be in a
different order than the docshell tree.  However, given the typically small
number of docshells, I think this will be ok.

The testcase still breaks on step 9, but I think it's for a different reason.

Comment 5

[Darin Fisher]

Comment on attachment 190608 [details] [diff] [review]
patch, part 1

Yeah, this looks great!  r=darin

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 190608 [details] [diff] [review]
patch, part 1

Requesting approval... these changes are _not_ localized to fastback, but I'm
fairly confident that they're correct.

Comment 7

[Brian Ryner (not reading)]

Attachment: more complete fix

So, in addition to not updating mOSHE on the toplevel docshell when cloning the
history tree, we also weren't updating it when you go back/forward in a frame. 
This patch makes it so that any time a subframe navigation happens, we walk the
history tree and update each corresponding docshell to point to the right entry
in the _new_ history tree.  The tree-walking code is shared between the cloning
and navigating cases.

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #7)
> Created an attachment (id=191377) [edit]
> more complete fix
> 

This is missing a simple null check, which I've since added.  Change:

+    if (aShell == ignoreShell)
+        return NS_OK;

to

+    if (!aShell || aShell == ignoreShell)
+        return NS_OK;

in nsDocShell::SetChildHistoryEntry.

Comment 9

[Darin Fisher]

Comment on attachment 191377 [details] [diff] [review]
more complete fix

>Index: docshell/base/nsDocShell.cpp

>+void
>+nsDocShell::SwapHistoryEntries(nsISHEntry *aOldEntry, nsISHEntry *aNewEntry)
>+{
>+    if (aOldEntry == mOSHE)
>+        mOSHE = aNewEntry;
>+
>+    if (aOldEntry == mLSHE)
>+        mLSHE = aNewEntry;
>+}

So, in some cases it is true that mOSHE == mLSHE such that this code
needs to do two checks instead of a single check (i.e., "else if" 
would be wrong)?


>+nsDocShell::SetHistoryEntry(nsCOMPtr<nsISHEntry> *aPtr, nsISHEntry *aEntry)
...
>+    newRootEntry = temp;
>+
>+    if (newRootEntry) {

It might be cleaner to do an early return here if !newRootEntry.
That way you reduce indentation.


>+        nsCOMPtr<nsISHEntry> oldRootEntry = *aPtr;
>+        temp = nsnull;
>+        while (oldRootEntry) {
>+            temp = oldRootEntry;
>+            temp->GetParent(getter_AddRefs(oldRootEntry));
>+        }
>+
>+        oldRootEntry = temp;

This looks like it could be a small subroutine since it is
coded twice.


>+                nsresult rv = SetChildHistoryEntry(oldRootEntry, rootDocShell,
>+                                                   0, &data);
>+                // XXX do something with rv

how about a debug assertion at least?


otherwise looks good, r=darin

Comment 10

[Brian Ryner (not reading)]

Attachment: patch w/ darin's comments addressed

Comment 11

[Brian Ryner (not reading)]

this is checked in.

Comment 12

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Hi Boris, here's another docshell test for you to review, as well as a question.

It seems as though when navigating inside an iframe, that the iframe page that you're navigating away from is not bfcached.  However, when you navigate away from the iframe parent, both the parent and the iframe content page are bfcached.  Is this correct?

In the test I've marked these cases with comments for the .persisted property of the pagehide event:  // why is this false?

Once I know the answer, I'll update the comments in the test.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> It seems as though when navigating inside an iframe, that the iframe page that
> you're navigating away from is not bfcached.  However, when you navigate away
> from the iframe parent, both the parent and the iframe content page are
> bfcached.  Is this correct?

Yep.  We don't bfcache subframe navigations at the moment.

The test looks fine, except you should reset the bfcache pref to the default value after you're done with the test.  Probably save the value it has when the test starts and reset it afterwards?  Ideally enableBFCache would handle that automatically somehow.

Comment 14

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case v2

Thanks Boris.  enableBFCache() now stores the original value of max_total_viewers, and restores that value when the test calls finish().

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 388273 [details] [diff] [review]
mochitest test case v2

r=bzbarsky.  Sorry for the ridiculous lag here.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/7d8393d63044 with that test.