312117 - target="_top" links in an iframe are invalidated with bfcache on (or off)

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Adam Guthrie]

If a link within an <iframe> has target="_top" and one views that link, then
goes back to the page with the iframe, links with target="_top" in that iframe
will not open in the current, full window, but rather within the iframe.

Steps to reproduce:
1. Open up the testcase and click the link to Slashdot.
2. Go back to the testcase without clicking any other links.
3. Click the Slashdot link again.

The Slashdot homepage is loaded within the iframe when it should be loaded into
the current window/tab as it did to begin with. This is not an issue with
max_viewers set to 0.

Comment 1

[Adam Guthrie]

Attachment: content of iframe

Comment 2

[Adam Guthrie]

Attachment: testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Why am I your default fall man?  bfcache is all bryner.

bryner, it sounds like we're not setting the right tree owner or parent on those
docshells or something?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah.  

(gdb) frame
#0  nsDocShell::FindItemWithName (this=0x88efa28, aName=0xbfffe968, aRequestor=0x0, 
    aOriginalRequestor=0x88efac4, _retval=0xbfffe7d0)
    at ../../../mozilla/docshell/base/nsDocShell.cpp:2074
2074                GetSameTypeRootTreeItem(getter_AddRefs(foundItem));
(gdb) p mParent
$2 = (nsDocLoader *) 0x0

That's not good at all -- some of our security checks use the docshell
hierarchy, and you can do things to parentless docshells that you can't do to
ones with parents.

Comment 5

[Brian Ryner (not reading)]

This is a recent regression from sicking's checkin for bug 292962.  The child
shells list in the SHEntry is getting cleared out by SetContentViewer(null) in
RestoreFromHistory, but we want to use the list of child shells after that.

However, this problem should not happen on the branch, because the lower-risk
patch we landed there does not call DropPresentationState from SetContentViewer.
 Re-setting the blocking1.8rc1 flag since I don't think this impacts the branch.

Comment 6

[Brian Ryner (not reading)]

I just verified on a branch build that the problem doesn't happen.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Removing nomination since this doesn't happen on branch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er... didn't we just back out bug 292962 from the branch altogether?  And aren't
we planning to reland it there now that we have bug 311269 fixed?  Certainly bug
292962 is still blocking1.8rc1+.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, 292962 was backed out alltogether. And I don't think that the part that did
land there would have caused this bug (can't tell for sure until I have looked
into this)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

*** Bug 313166 has been marked as a duplicate of this bug. ***

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm upping this to critical severity given the role that parent chains play in
security checks.

Comment 12

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051013 SeaMonkey/1.1a]
(nightly) (W98SE)
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051021 SeaMonkey/1.1a]
(nightly) (W98SE)

(I see this bug/testcase, and)
I believe I'm seeing this/same bug with regular frameset/frames on ("private")
<www.battle-arenas.net>.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> (I see this bug/testcase, and)
> I believe I'm seeing this/same bug with regular frameset/frames on ("private")
> <www.battle-arenas.net>.

Bug started with
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051008 SeaMonkey/1.1a] (nightly) (W98SE)

(testcase and wBAn site) Occurs with
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051022 SeaMonkey/1.1a] (nightly) (W98SE)
even with |user_pref("browser.sessionhistory.max_viewers", 0);|

Comment 14

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051030 SeaMonkey/1.1a] (nightly) (W98SE)

Help, this is still preventing me from using SMv1.1a Trunk builds :-(
(upping Severity to Blocker)

Comment 15

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051104 SeaMonkey/1.1a] (nightly) (W98SE)

Blocker weekly report: still there.

Comment 16

[Brian Ryner (not reading)]

Attachment: fix

Get all of the state off the SHEntry before we clear its content viewer.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202049 [details] [diff] [review]
fix

>Index: nsDocShell.cpp
>     // Reattach to the window object.
>     rv = mContentViewer->Open(windowState, mLSHE);

Please add asserts after this line that:

1)  mLSHE's refresh URI list is null.
2)  mLSHE->ChildShellAt(0) returns null.

With those, sr=bzbarsky

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 202049 [details] [diff] [review]
fix

Thanks for taking care of this!

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Has somebody double-checked that this doesn't happen on the 1.8 branch with what's now landed there?

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Has somebody double-checked that this doesn't happen on the 1.8 branch with
> what's now landed there?

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051107 SeaMonkey/1.0b] (release) (W98SE)

Never (since comment 12) had this bug, per testcase or wBAn site.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051108 SeaMonkey/1.5a] (nightly) (W98SE)

(still buggy, per testcase)

Comment 21

[Brian Ryner (not reading)]

checked in

Comment 22

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051109 SeaMonkey/1.5a] (nightly) (W98SE)

V.Fixed, per testcase.

Comment 23

[Serge Gautherie (:sgautherie)]

*** Bug 314252 has been marked as a duplicate of this bug. ***