Incorrect page displayed in iframe with bfcache

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
Document Navigation
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Adrian Yee, Assigned: Brian Ryner (not reading))

Tracking

({testcase})

Trunk
mozilla1.8beta4
x86
Linux
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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
Blocks: 274784
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 2

13 years ago
Sounds serious, targetting for beta.
Assignee: nobody → bryner
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Comment 3

13 years ago
Created attachment 190435 [details] [diff] [review]
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.
Attachment #190435 - Flags: superreview?(cbiesinger)
Attachment #190435 - Flags: review?(darin)
(Assignee)

Updated

13 years ago
Attachment #190435 - Flags: superreview?(cbiesinger)
Attachment #190435 - Flags: review?(darin)
(Assignee)

Comment 4

13 years ago
Created attachment 190608 [details] [diff] [review]
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.
Attachment #190435 - Attachment is obsolete: true
Attachment #190608 - Flags: review?(darin)

Comment 5

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

Yeah, this looks great!  r=darin
Attachment #190608 - Flags: review?(darin) → review+
(Assignee)

Comment 6

13 years ago
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.
Attachment #190608 - Flags: approval1.8b4?
Flags: blocking1.8b4+

Updated

13 years ago
Attachment #190608 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 7

13 years ago
Created attachment 191377 [details] [diff] [review]
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.
Attachment #190608 - Attachment is obsolete: true
Attachment #191377 - Flags: review?(darin)
(Assignee)

Comment 8

13 years ago
(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

13 years ago
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
Attachment #191377 - Flags: review?(darin) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 191532 [details] [diff] [review]
patch w/ darin's comments addressed
Attachment #191377 - Attachment is obsolete: true
(Assignee)

Comment 11

13 years ago
this is checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Depends on: 304639
Depends on: 305137

Updated

10 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
Created attachment 387981 [details] [diff] [review]
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.
Attachment #387981 - Flags: review?(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.
Created attachment 388273 [details] [diff] [review]
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().
Attachment #387981 - Attachment is obsolete: true
Attachment #388273 - Flags: review?(bzbarsky)
Attachment #387981 - Flags: review?(bzbarsky)
Attachment #388273 - Flags: review?(bzbarsky) → review+
Comment on attachment 388273 [details] [diff] [review]
mochitest test case v2

r=bzbarsky.  Sorry for the ridiculous lag here.  :(
You need to log in before you can comment on or make changes to this bug.