222191 - can't set frame location if frame name is "sidebar"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[ralph]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a) Gecko/20031014
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a) Gecko/20031014

Setting the file location of an HTML frame with JavaScript fails if the frame's
name is "sidebar." That is, this:

top.sidebar.location = "sidebar.php";

fails with this error:

Error: uncaught exception: [Exception... "Cannot modify properties of a
WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" 
location: "JS frame :: http://harborresearch.com/ct2/ :: populateFrames :: line
219"  data: no]

Changing the name of the frame to "side" fixes it. Thus, it seems that the word
"sidebar" is incorrectly interpreted as a reserved word. This bug does NOT exist
in the latest Mac/Win IE, Navigator, and Mac-specific browsers like Safari and
Camino. Strangely, this bug also does NOT exist in the latest Win Firebird
(haven't tested the latest Win Mozilla, but presumably the Firebird testing
covers that).

Reproducible: Always

Steps to Reproduce:
1.Create a frameset where a frame is named "sidebar".
2.Attempt to set the location of that frame with top.sidebar.location = "file.html"
3.

Actual Results:  
Failure to set frame file location. JavaScript console reports this:

Error: uncaught exception: [Exception... "Cannot modify properties of a
WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" 
location: "JS frame :: http://harborresearch.com/ct2/ :: populateFrames :: line
219"  data: no]

Expected Results:  
Load the file in the frame.

Comment 1

[Boris Zbarsky [:bzbarsky]]

window.sidebar is the Mozilla sidebar.  I suppose we could not expose that to
scripts, but some scripts rely on being able to get to it...

In Firebird, there is currently no sidebar support, hence no window.sidebar object.

Using top.frames['sidebar'] should work in all browsers and not depend on
particular properties of the window objects existing or not existing...

Comment 2

[Jesse Ruderman]

This also breaks logging into https://onlinebanking.huntington.com/ .  If you
click "Enter", nothing happens.

javascript:alert(window.top.frames["sidebar"]) 
shows 
[xpconnect wrapped (nsISupports, nsISidebar, nsIClassInfo)]
so the site can't fix itself by using window.top.frames["sidebar"] instead of
window.top.sidebar.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Caillon, you broke this.  We need to keep window.sidebar replaceable or rename
it to something that won't conflict with web pages (window.mozillaSidebar,
window.__sidebar, whatever).

Comment 6

[Boris Zbarsky [:bzbarsky]]

This should block 1.6, in my opinion....

Comment 7

[Boris Zbarsky [:bzbarsky]]

By the way, this is also broken in current FB builds, now that that patch landed.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: This fixes the problem

So, simply re-inserting the |static jsval sSidebar_id| won't work because now
window.sidebar is explicitly registered with the script namespace manager,
which will cause GlobalResolve() to resolve the native sidebar.  Move this down
to after we check for named child frames.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 135975 [details] [diff] [review]
This fixes the problem

+jsval nsDOMClassInfo::sSidebar_id	   = JSVAL_VOID;

No need for this, the (readonly) replaceable stuff only matters for defined DOM
properties, "sidebar" is now simply a JS property on the global object.

r=jst if you take out sSidebar.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Oops, right, I had forgotten to take that bit out.  It's zapped locally.

I also made a small comment update:
+      // Call GlobalResolve() after the call to
+      // nsIDocShellTreeItem::FindChildWithName() so that named child

FindChildWithName() is on nsIDocShellTreeNode, not Item.  I removed the
qualifier completely, since it forces a really early line break, and the reader
should be able to figure it out anyway ;-)

Eyeing beta...

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 135975 [details] [diff] [review]
This fixes the problem

Doesn't this mean that someone can for example name a frame "Node" and that
window.Node will now refer to the frame instead of the DOM class?

Comment 12

[Bill Gianopoulos [:WG9s]]

I can verify that the patch fixes the problem originally reported by me in Bug #
225453.  With this patch installed you can once again configure a Cisco VPN
concentrator using Mozilla Firebird as your Browser.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Peterv, yes it will clobber window.Node, etc.  But that really should not
matter.  Anyone who is doing that is clearly writing code for IE which does not
even provide window.Node to begin with.  Plus, this is too big of an edge case I
think.  I don't think that we should worry at all about this case, and just give
frame names priority over DOM classes (note that the ECMA standard classes
should still always resolve correctly to the prototypes).  Anyone who knowingly
names a frame with the name of a DOM class prototype name they intend to use
later on deserves what they get, IMO.

Comment 14

[Asa Dotzler [:asa]]

Not going to hold beta for this (though we'd consider approving for beta if it
gets reviews in time) but we should get this before we ship final.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 135975 [details] [diff] [review]
This fixes the problem

r=me with the sSidebar_id zappage.

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 135975 [details] [diff] [review]
This fixes the problem

Re-requesting 1.6b approval -- this can go in now.

/be

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 135975 [details] [diff] [review]
This fixes the problem

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.  Fixed.

Comment 19

[Fabian Guisset]

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