295782 - [FIXr]Consider separating out the deepness and auto-unwrapping of XPCNativeWrappers

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

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 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Same but with enough context...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff -w

Comment 5

[Brendan Eich [:brendan]]

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 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to Brendan's comments

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

Comment 8

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Brendan Eich [:brendan]]

(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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Brendan Eich [:brendan]]

<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

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to remaining comments

Brendan, want to give this a last once-over?

Requesting 1.8b3 approval...

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to nits

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b3.