307632 - Can't extend Selection Class anymore with Firefox 1.5b1

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Christian Stocker]

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.

Comment 1

[Bob Clary [:bc] (inactive)]

bz, what are the conditions (if any) that people can set "expando" properties on
the dom in these days of XPC Wrappers ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to comment #3)

I would say in general as this is a common use case.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Johnny Stenback (:jst)]

Attachment: Use the current inner scope when wrapping natives in an outer window's scope.

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).

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 195818 [details] [diff] [review]
Use the current inner scope when wrapping natives in an outer window's scope.

r=mrbkap

Comment 9

[Brendan Eich [:brendan]]

This bug is must-fix, it restores DOM compatibility that goes back over seven
years (almost nine years for DOM level 0 objects).

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Johnny Stenback (:jst)]

(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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  The precreate hooks on location and navigator are what makes this work.
 Good.

Comment 13

[Johnny Stenback (:jst)]

Fixed on the branch too.