542428 - Make wrappers aware of each other

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, wrappers assume that they're the only wrappers in the world, which is not correct. There are multiple wrappers and they need to talk to each other to be complete. So here's a patch that does that.

jst, I made some changes to the XPCNativeWrapper and XPCSafeJSObjectWrapper constructors that you haven't seen yet.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 2

[chris hofmann]

blocking requested on 1.9.3 per https://bugzilla.mozilla.org/show_bug.cgi?id=537838#c1 and 2

Comment 3

[chris hofmann]

any thoughts on when this might hit the trunk for people to play with?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated

This includes fixes for a mochichrome test and session store (why are they still using evalInSandbox?).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 428351 [details] [diff] [review]
updated

- In XPC_COW_GetOrSetProperty():

   JSBool ok = isSet
               ? JS_SetPropertyById(cx, wrappedObj, interned_id, vp)
               : JS_GetPropertyById(cx, wrappedObj, interned_id, vp);
   if (!ok) {
     return JS_FALSE;
   }
 
-  return XPC_COW_RewrapForContent(cx, obj, vp);
+  return RewrapForContent(cx, obj, vp);

Do we really need to do this when setting a property? I guess if it's cheap enough to do, or if we think we have native setters that change *vp we should leave it.

- In WrapJSValue():

   if (JSVAL_IS_PRIMITIVE(val)) {
     *rval = val;
   } else {
+    if (!RewrapObject(cx, STOBJ_GET_PARENT(obj), JSVAL_TO_OBJECT(val), SJOW,
+                      rval)) {
+      return JS_FALSE;
+    }
     // Construct a new safe wrapper. Note that it doesn't matter what
     // parent we pass in here, the construct hook will ensure we get
     // the right parent for the wrapper.
-    JSObject *safeObj =
-      ::JS_ConstructObjectWithArguments(cx, &SJOWClass.base, nsnull,
-                                        nsnull, 1, &val);

The above comment seems out of place now.

+  if (caller != NONE && (desc.attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
+    NS_ASSERTION(caller == COW, "bad caller");

You mention that this should go...

r=jst with that looked into.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> (From update of attachment 428351 [details] [diff] [review])
> - In XPC_COW_GetOrSetProperty():
> 
>    JSBool ok = isSet
>                ? JS_SetPropertyById(cx, wrappedObj, interned_id, vp)
>                : JS_GetPropertyById(cx, wrappedObj, interned_id, vp);
>    if (!ok) {
>      return JS_FALSE;
>    }
> 
> -  return XPC_COW_RewrapForContent(cx, obj, vp);
> +  return RewrapForContent(cx, obj, vp);
> 
> Do we really need to do this when setting a property? I guess if it's cheap
> enough to do, or if we think we have native setters that change *vp we should
> leave it.

I want to leave this in because it seems safer to me. If the setter does return some random value, it seems better to over-wrap it than under-wrap it.

I've addressed the other comments, and fixed a mochitest failure.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Unfortunately, interdiff doesn't work on the two patches and I don't have a good way of generating one.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Re comment 7, you can always add an assertion in addition to handling it. Later we can decide if we want to remove the assertion or the code to handle the situation.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Additional fix

This is an additional patch that is needed. We can't tell ourselves to create an XPCNativeWrapper around a non-WrappedNative. Doing so causes us to call into XPCWrappedNative::GetAndMorphWrappedNativeOfJSObject (phew!), which returns null (expected for an Array) and causes us to return false without throwing because we thought that *morphing* was the thing that failed (which does set an exception).

Finding this was ... tricky.

The one thing I'm not sure about is whether I should unchain the ternary operators in favor of if statements (I might just do this anyway).

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/8dc04a747e92