Object.isExtensible() returns true for some non-extensible wrappedNative objects

RESOLVED FIXED

Status

()

Core
Geolocation
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Mark Bessey, Unassigned)

Tracking

14 Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/534.57.2 (KHTML, like Gecko) Version/5.1.7 Safari/534.57.2

Steps to reproduce:

Our framework tries to add some additional properties to JavaScript events while routing them internally. In the case of Geolocation events, the attempt to add a property fails, even though object.isExtensible() returns true for these objects.



Actual results:

From the Firebug console:

>>> Object.isExtensible(e)
true

>>> e.origin=this;
[Exception... "Cannot modify properties of a WrappedNative" nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" location: "JS frame :: :: <TOP_LEVEL> :: line 1" data: no] { message="Cannot modify properties of a WrappedNative", result=2153185332, name="NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN", more...}

>>> Object.prototype.toString.call(e)
"[object XPCWrappedNative_NoHelper]"



Expected results:

Object.isExtensible() should return FALSE for objects that can't have properties added to them.

Updated

6 years ago
Component: Untriaged → Geolocation
Product: Firefox → Core

Comment 1

6 years ago
This sounds more like XPConnect territory rather than anything specific to the Geolocation code.
Component: Geolocation → XPConnect
Actually, this is sorta geolocation.  It uses DOM_DEFAULT_SCRIPTABLE_FLAGS in its classinfo, which do not include nsIXPCScriptable::USE_JSSTUB_FOR_SETPROPERTY.  I wonder whether we should add that flag there...

Or better yet, move geolocation to the WebIDL bindings, which don't have issues like this.  ;)

Oh, and all that said, isExtensible being true is still correct here, per the letter of the spec.  Welcome to the wonderful world of host objects and proxies.
Component: XPConnect → Geolocation
Actually, DOM_DEFAULT_SCRIPTABLE_FLAGS does include DOM_BASE_SCRIPTABLE_FLAGS, which includes USE_JSSTUB_FOR_SETPROPERTY.

There also aren't any geolocation event objects.

So that brings us back to the real question:  Mark, which _exact_ objects are you working with here?  That sort of matters....
Mark: and in particular, what would be ideal here is an actual web page that shows the problem.
(Reporter)

Comment 5

6 years ago
Sorry, that's a sloppy mixing of terminology on my part. The objects in question are the parameters passed to the Geolocation callbacks. 

So, for example, this throws an exception:

navigator.geolocation.getCurrentPosition(function(position) {  
   // add a property to the position
   position.myProperty=true;
   do_something(position);  
});

I'll attach an example in a couple of minutes.
Mark, that's plenty to reproduce, thanks.

So what we get here is an XPCWrappedNative_NoHelper.  The actual object it's wrapping is an nsXPTCStubBase(!).  That at least explains why we get the exception.  Now why do we have an nsXPTCStubBase here?  What sort of object should get passed to this callback?
The nsXPTCStubBase unsurprisingly wraps an nsXPCWrappedJS around some Object.

The nsXPCWrappedJS thinks it's implementing nsIDOMGeoPosition.

Why is that a vanilla Object and not an nsGeoPosition?
jdm says it's due to http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#56 which seems plausible.

So either that should use an nsGeoPosition or we need to make xpconnect allow expandos here in some fashion.  Implementing nsIXPCScriptable is of course not an option here.

We _could_ make DOM_OBJECT imply adding expandos is OK, though.  Maybe that's the way to go.
(In reply to Boris Zbarsky (:bz) from comment #8)

> We _could_ make DOM_OBJECT imply adding expandos is OK, though.  Maybe
> that's the way to go.

I don't think so. Allowing expandos, at the bare minimum, means:

1 - We need a PreCreate hook, otherwise we'll get one XPCWN per scope and the expandos will differ depending on what scope is examining the object.

2 - The object needs to implement nsWrapperCache, otherwise the expandos will randomly disappear.
Lots of existing DOM objects allow expandos without doing either of those things.   Why are we holding this case to a higher standard?

(And note that those two things aren't even enough to do this "right"; you also need to do wrapper preservation.)
Blake, thoughts on last paragraph of comment 8?
(In reply to Boris Zbarsky (:bz) from comment #11)
> Blake, thoughts on last paragraph of comment 8?

Sorry for the delay, I thought I'd commented here.... I'm not terribly happy with the idea of adding a bunch of objects that will randomly lose their expandos, but if we're OK with that, as comment 10 implies, it's pretty clear that DOM objects have to behave more like regular objects than XPCOM objects, so we should probably do the last paragraph of comment 8.
Also, note that a lot of this stuff is totally held together by a shoestring right now, because of bug 781476. In particular, it's now possible to have two identity objects in a given scope for a single underlying object, which is super lame.
It looks like we no longer directly expose raw JS object WifiGeoPositionObject instances from NetworkGeolocationProvider.js to web pages.  I bet this got fixed in bug 850442, since we now expose a Web IDL Position object instead, which wraps the underlying thing.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Depends on: 850442
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.