Closed Bug 307632 Opened 19 years ago Closed 19 years ago

Can't extend Selection Class anymore with Firefox 1.5b1

Categories

(Core :: XPConnect, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: chregu, Assigned: dbradley)

References

()

Details

(Keywords: fixed1.8)

Attachments

(1 file)

User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 Extending the Selection class with new prototypes seems not to work anymore in Firefox 1.5b1: Selection.prototype.foo = function() { alert("foo called"); } did work with 1.0.x (and everything before), but now I get Error: sel.foo is not a function Now my editor (bxe.oscom.org) doesn't work anymore, which heavily relies on extending the selection class. Others (like mozile) will have the same problem, I assume. Reproducible: Always Steps to Reproduce: 1. Go to URL http://trash.chregu.tv/selection.prototype.bug.html 2. Click on click here 3. Actual Results: Nothing. Error in JS Console Expected Results: An alert box with "foo called" should appear.
bz, what are the conditions (if any) that people can set "expando" properties on the dom in these days of XPC Wrappers ?
Component: JavaScript Engine → XPConnect
Summary: Can't extend Selection Class anymore with Firefox 1.5b1 → Can't extend Selection Class anymore with Firefox 1.5b1
Assignee: general → dbradley
QA Contact: general → pschwartau
There are no XPCNativeWrappers involved (but in any case, XPCNativeWrapper expand stuff is documented at http://developer.mozilla.org/en/docs/XPCNativeWrapper#Setting_.22expando.22_properties_on_XPCNativeWrapper -- let me know if that's not clear enough, ok?). This bug is almost certainly splitwindow fallout. The basic issue is that the prototype of what getSelection() returns is something that lives on the outer window, while the code in the page has changed Selection.prototype on the _inner_ window. This happens because the getSelection call is made on the outer window and hence in the outer XPConnect scope, and so that's the scope that XPConnect uses when it does NativeInterface2JSObject. Now selection _is_ something that lives on the outer window. Sorta. It lives in the presshell, but we go through the docshell to get the presshell for some odd reason. What would make more sense to me is to forward selection-getting to the inner and to just get the presshell off mDocument.... And then we can happily change things so selection is wrapped in the inner window scope (via classinfo). Right?
Blocks: splitwindows
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b5?
OS: MacOS X → All
Hardware: Macintosh → All
Though that approach really will not help with other things (eg window.location, window.navigator) that _do_ live on the outer window and hence can't be wrapped in the inner's scope. Do we want to just fix selection, or try to come up with something that works in general?
(In reply to comment #3) I would say in general as this is a common use case.
Could we perhaps define the standard classes on the proto of the window, not on the window itself? Since the two windows share the proto chain, maybe that will work?
This fixes this bug by making us always wrap all DOM things in an inner scope. We use the current inner here since that's our best guess here. This way we won't end up with different wrappers for the selection object when using "selection" vs "window.selection" (where in the latter case the scope we wrap the selection object in is the window's scope, i.e. the outer).
Attachment #195818 - Flags: superreview?(brendan)
Attachment #195818 - Flags: review?(mrbkap)
Comment on attachment 195818 [details] [diff] [review] Use the current inner scope when wrapping natives in an outer window's scope. sr=me ahead of mrbkap, he better not embarrass me ;-). /be
Attachment #195818 - Flags: superreview?(brendan) → superreview+
Comment on attachment 195818 [details] [diff] [review] Use the current inner scope when wrapping natives in an outer window's scope. r=mrbkap
Attachment #195818 - Flags: review?(mrbkap) → review+
Attachment #195818 - Flags: approval1.8b5?
This bug is must-fix, it restores DOM compatibility that goes back over seven years (almost nine years for DOM level 0 objects). /be
Flags: blocking1.8b5? → blocking1.8b5+
Er... so how does wrapping in the inner scope work for things that outlive the inner window? Won't they basically keep the inner window alive while they're alive? And always have its permissions, which is worse, I think? I'm thinking things like window.location, window.navigator, etc.
(In reply to comment #10) > Er... so how does wrapping in the inner scope work for things that outlive the > inner window? Won't they basically keep the inner window alive while they're > alive? And always have its permissions, which is worse, I think? > > I'm thinking things like window.location, window.navigator, etc. window.location and navigator, and DOM nodes, and window objects already have pre-create hooks much like this one that does the exact same thing (well, window.location and navigator's pre-create hooks actually do the opposite). There's no security problems there AFAICT, the principals remain unchanged now, unlike before the split window landing, and besides, that'd only be a problem if a site could inject code onto an object and get it to inherit the objec's principals, which AFAIK is not possible. Brendan, correct me if I'm wrong here. This landed on the trunk btw, marking FIXED.
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking1.8b5+ → blocking1.8b5?
Resolution: --- → FIXED
Ah, ok. The precreate hooks on location and navigator are what makes this work. Good.
Flags: blocking1.8b5? → blocking1.8b5+
Attachment #195818 - Flags: approval1.8b5? → approval1.8b5+
Fixed on the branch too.
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: