Closed Bug 222191 Opened 21 years ago Closed 21 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.6beta

People

(Reporter: ralph, Assigned: caillon)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
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...
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: MacOS X → All
Hardware: Macintosh → All
*** Bug 225838 has been marked as a duplicate of this bug. ***
*** Bug 225453 has been marked as a duplicate of this bug. ***
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).
Assignee: general → caillon
This should block 1.6, in my opinion....
Flags: blocking1.6?
By the way, this is also broken in current FB builds, now that that patch landed.
Keywords: regression
Attached patch This fixes the problem (deleted) — Splinter Review
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.
Attachment #135975 - Flags: superreview?(peterv)
Attachment #135975 - Flags: review?(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.
Attachment #135975 - Flags: review?(jst) → review+
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...
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.6beta
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?
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.
Flags: blocking1.6b?
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.
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.
Flags: blocking1.6b?
Flags: blocking1.6b-
Flags: blocking1.6?
Flags: blocking1.6+
Comment on attachment 135975 [details] [diff] [review] This fixes the problem r=me with the sSidebar_id zappage. /be
Attachment #135975 - Flags: superreview?(peterv) → superreview+
Comment on attachment 135975 [details] [diff] [review] This fixes the problem Re-requesting 1.6b approval -- this can go in now. /be
Attachment #135975 - Flags: approval1.6b?
Comment on attachment 135975 [details] [diff] [review] This fixes the problem a=asa (on behalf of drivers) for checkin to 1.6beta
Attachment #135975 - Flags: approval1.6b? → approval1.6b+
Checked in. Fixed.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 99571 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: