Closed Bug 301397 Opened 15 years ago Closed 15 years ago

Incorrect page displayed in iframe with bfcache

Categories

(Core :: DOM: Navigation, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: brewt-bugzilla.mozilla.org, Assigned: bryner)

References

()

Details

(Keywords: testcase)

Attachments

(2 files, 4 obsolete files)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Sounds serious, targetting for beta.
Assignee: nobody → bryner
Target Milestone: --- → mozilla1.8beta4
Attached patch patch, part 1 (obsolete) — Splinter Review
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)
Attachment #190435 - Flags: superreview?(cbiesinger)
Attachment #190435 - Flags: review?(darin)
Attached patch patch, part 1 (obsolete) — Splinter Review
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 on attachment 190608 [details] [diff] [review]
patch, part 1

Yeah, this looks great!  r=darin
Attachment #190608 - Flags: review?(darin) → review+
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+
Attachment #190608 - Flags: approval1.8b4? → approval1.8b4+
Attached patch more complete fix (obsolete) — Splinter Review
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)
(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 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+
Attachment #191377 - Attachment is obsolete: true
this is checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 304639
Depends on: 305137
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
Attached patch mochitest test case (obsolete) — Splinter Review
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.
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.