I was doing some thinking about this, and it seems to me that we're really overloading the "deep" flag in a rather poor way when we decide to auto-unwrap. For example, if chrome that's not using auto-wrapping creates a manual wrapper with no string args, it effectively gets a shallow wrapper. So I think we want to add another flag to native wrappers and flag all manually created wrappers and all wrappers created by a manual wrapper's ReWrapIfDeepWrapper with it. This will do the right thing for automatically created wrappers, and will allow extension authors to selectively enable deep wrapping for some object if they need it. Thoughts?

We need to decide on something for 1.8b3, imo, since we want to document the behavior of XPCNativeWrapper....

This adds a slot for storing whether the wrapper is explicit and ensures that deep explicit wrappers lead to explicit wrappers. With this patch, the mNativeWrapper in an XPCWrappedNative is the unique non-explicit (deep, all non-explicit wrappers are deep) wrapper, if any.

Comment on attachment 184861 [details] [diff] [review] Same but with enough context... > JSExtendedClass XPCNativeWrapper::sXPC_NW_JSClass = { > // JSClass (JSExtendedClass.base) initialization > { "XPCNativeWrapper", > JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS | >- JSCLASS_NEW_RESOLVE | JSCLASS_HAS_RESERVED_SLOTS(1) | >+ JSCLASS_NEW_RESOLVE | JSCLASS_HAS_RESERVED_SLOTS(2) | Why not use one slot containing an INT jsval, containing flag bits? > static JSBool > ShouldBypassNativeWrapper(JSContext *cx, JSObject *obj) > { >- jsval deep = JS_FALSE; >+ jsval explicitWrapper = JS_TRUE; Yikes, this must have happened while I was away. jsval type needs JSVAL_* constants, not JS_*. >- ::JS_GetReservedSlot(cx, obj, 0, &deep); This will always set deep, so there's actually no need to initialize, but since you are ignoring the return value... ah, that's why someone added the bogus JS_FALSE initializer to the declaration. But it turns out that the only false return from JS_GetReservedSlot is for the case where the slot is out of range, and that can't happen. If it did fail, however, we would want to suppress the error-as-exception it threw. So it's just silly to worry about it failing here. How about reverting to something like what I originally wrote here? Or was there a bug to fix with that code? >- return deep == JSVAL_TRUE && (ahhh, JSVAL_TRUE, not JS_TRUE. ;-) >+ ::JS_GetReservedSlot(cx, obj, SLOT_EXPLICIT, &explicitWrapper); >+ return explicitWrapper == JSVAL_FALSE && > !(::JS_GetTopScriptFilenameFlags(cx, nsnull) & JSFILENAME_SYSTEM); > } > > #define XPC_NW_BYPASS_BASE(cx, obj, code) \ > JS_BEGIN_MACRO \ > if (ShouldBypassNativeWrapper(cx, obj)) { \ > XPCWrappedNative *wn_ = XPCNativeWrapper::GetWrappedNative(cx, obj); \ > if (!wn_) { \ >@@ -204,34 +207,56 @@ XPC_NW_DelProperty(JSContext *cx, JSObje > > return ThrowException(NS_ERROR_XPC_SECURITY_MANAGER_VETO, cx); > } > > static JSBool > RewrapIfDeepWrapper(JSContext *cx, JSObject *obj, jsval v, jsval *rval) > { > jsval deep; >- if (!::JS_GetReservedSlot(cx, obj, 0, &deep)) { >+ if (!::JS_GetReservedSlot(cx, obj, SLOT_DEEP, &deep)) { > return JS_FALSE; > } > > // Re-wrap non-primitive values if this is a deep wrapper, i.e. >- // if (deep == JSVAL_TRUE). Note that just using GetNewOrUsed >- // on the return value of GetWrappedNativeOfJSObject will give >- // the right thing -- the unique deep wrapper associated to >- // wrappedNative. >+ // if (deep == JSVAL_TRUE). > if (deep == JSVAL_TRUE && !JSVAL_IS_PRIMITIVE(v)) { >- JSObject *nativeObj = JSVAL_TO_OBJECT(v); >- XPCWrappedNative* wrappedNative = >- XPCWrappedNative::GetWrappedNativeOfJSObject(cx, nativeObj); >- if (!wrappedNative) { >- return ThrowException(NS_ERROR_INVALID_ARG, cx); >+ jsval explicitWrapper; >+ if (!::JS_GetReservedSlot(cx, obj, SLOT_EXPLICIT, &explicitWrapper)) { >+ return JS_FALSE; >+ } Here it looks again as though proving obj is of the right class (which has one reserved slot, which holds two flags), we can simplify the code to (a) minimize JS_GetReservedSlot calls; (b) avoid error returns after the one such call that's left. >+ } else { >+#ifdef DEBUG_XPCNativeWrapper >+ printf("Rewrapping for deep explicit wrapper\n"); >+#endif >+ // |obj| is an explicit deep wrapper. We want to construct >+ // |another explicit deep wrapper for |v|. >+ wrapperObj = >+ ::JS_ConstructObjectWithArguments(cx, XPCNativeWrapper::GetJSClass(), >+ nsnull, ::JS_GetGlobalObject(cx), Who keeps adding back these ::JS_GetGlobalObject(cx) calls for the |parent| formal param, that I removed a few patches ago? They're wrong. Is there a bad pattern nearby in xpconnect/src/? /be

Comment on attachment 184863 [details] [diff] [review] Diff -w > Why not use one slot containing an INT jsval, containing flag bits? Because I'm not thinking? ;) Will do. > Yikes, this must have happened while I was away. Actually, you checked in that code (bug 294960). And I reviewed it... :( > So it's just silly to worry about it failing here. OK, that works. > Here it looks again as though proving obj is of the right class I think that's provable, and I'll add an assert for good measure. > Who keeps adding back these ::JS_GetGlobalObject(cx) calls for the |parent| I added it just now, by copy-paste from what the code was doing earlier. Really, though, it totally doesn't matter what I pass here. See http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/XPCNativeWrapper.c pp#833 in the Ctor function. We have to do that anyway for |new XPCNativeWrapper()| calls from JS, so.... I can more clearly document this part if you like.

Brendan, jst, feel free to give this r+sr, whichever of you gets here first.

(In reply to comment #6) > > Yikes, this must have happened while I was away. > > Actually, you checked in that code (bug 294960). And I reviewed it... :( That was my evil twin, apparently -- the one who was awake when I was asleep at the keyboard ;-). Thanks for setting the record straight. > Really, though, it totally doesn't matter what I pass here. See > http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/XPCNativeWrapper.c > pp#833 in the Ctor function. We have to do that anyway for |new > XPCNativeWrapper()| calls from JS, so.... I can more clearly document this > part if you like. Right, it's ok to coerce parent in the ctor. But we shouldn't pass JS_GetGlobalObject(cx) to JS_{New,Construct}Object* in any event. Ok, looking at your new patch now. /be

Comment on attachment 184933 [details] [diff] [review] Patch updated to Brendan's comments >+ // |obj| is an explicit deep wrapper. We want to construct another >+ // explicit deep wrapper for |v|. Note that it really doesn't matter >+ // much what parent we pass here, since XPCNativeWrapperCtor will create >+ // a new object with correct parentage and the like anyway. I suppose we >+ // could just call XPCNativeWrapperCtor directly and not bother with >+ // JS_ConstructObjectWithArguments.... >+ wrapperObj = >+ ::JS_ConstructObjectWithArguments(cx, XPCNativeWrapper::GetJSClass(), >+ nsnull, ::JS_GetGlobalObject(cx), So just pass nsnull instead of ::JS_GetGlobalObject(cx) here. But: The reason to call JS_ConstructObjectWithArguments is exactly to find the class name in the scope chain, and seed __proto__ from className.prototype and __parent__ from className.__parent__. If we are always and everywhere overriding both proto and parent, then you could indeed call XPCNativeWrapperCtor on a new object of the native wrapper class. Why not tighten this up given all the code is local to this file? > JSBool isDeep = !hasStringArgs; >- if (!::JS_SetReservedSlot(cx, wrapperObj, 0, BOOLEAN_TO_JSVAL(isDeep))) { >+ jsuint flags = FLAG_EXPLICIT; >+ if (isDeep) { >+ flags |= FLAG_DEEP; >+ } I always prefer constant folding flags explicitly: jsuint flags = isDeep ? FLAG_DEEP | FLAG_EXPLICIT : FLAG_EXPLICIT; /be

> If we are always and everywhere overriding both proto and parent We are. We never access the |obj| passed to XPCNativeWrapperCtor, in fact. We create our own object. So if I were to call XPCNativeWrapperCtor by hand, I would just pass null for |obj|. In fact, I think I should add an |obj = nsnull| at the top of that method, just to make sure no one gets tempted to use |obj| for anything.

(In reply to comment #10) > > If we are always and everywhere overriding both proto and parent > > We are. We never access the |obj| passed to XPCNativeWrapperCtor, in fact. > We create our own object. I dimly recalled jst's original version being more parsimonious. It's wasteful to throw the in param |obj| away if the JS engine made one of the right class, but sometimes it is hard to avoid without major contortions. Is this such a case for sure? /be

> I dimly recalled jst's original version being more parsimonious. It was, but that lead to an incorrect proto chain (and leaks though entraining the chrome scope in the proto chain of XPCNativeWrappers). I suppose we could reuse the incoming |obj| if we made sure to reset its proto as well as its parent. But then we have to duplicate a bunch of proto-finding logic that JS_NewObject already does for us...

<bz> And I'll make this patch call Ctor directly <bz> with null obj <bz> That will prevent silly object construction in this (common) case r+sr=me with that change. /be

Brendan, want to give this a last once-over? Requesting 1.8b3 approval...

Comment on attachment 184950 [details] [diff] [review] Updated to remaining comments > // Re-wrap non-primitive values if this is a deep wrapper, i.e. >- // if (deep == JSVAL_TRUE). Note that just using GetNewOrUsed >- // on the return value of GetWrappedNativeOfJSObject will give >- // the right thing -- the unique deep wrapper associated to Nit: associated with? Or mapped to, but it's bidirectional, eh? >- // wrappedNative. >- if (deep == JSVAL_TRUE && !JSVAL_IS_PRIMITIVE(v)) { >+ // if (HAS_FLAGS(flags, FLAG_DEEP). >+ if (HAS_FLAGS(flags, FLAG_DEEP) && !JSVAL_IS_PRIMITIVE(v)) { >+ >+ if (HAS_FLAGS(flags, FLAG_EXPLICIT)) { >+#ifdef DEBUG_XPCNativeWrapper >+ printf("Rewrapping for deep explicit wrapper\n"); >+#endif >+ // |obj| is an explicit deep wrapper. We want to construct another >+ // explicit deep wrapper for |v|. Just call XPCNativeWrapperCtor by hand >+ // (passing null as the pre-created object it doesn't use anyway) so we >+ // don't have to create an object we never use. >+ >+ return XPCNativeWrapperCtor(cx, nsnull, 1, &v, rval); >+ } >+ >+#ifdef DEBUG_XPCNativeWrapper >+ printf("Rewrapping for deep non-explicit wrapper\n"); >+#endif >+ // Just using GetNewOrUsed on the return value of >+ // GetWrappedNativeOfJSObject will give the right thing -- the unique deep >+ // non-explicit wrapper associated to wrappedNative. Nits: s/non-explicit/implicit/ and same nit as above for "associated to". r+sr+a=me, thanks again. /be

Fixed for 1.8b3.