Closed Bug 542428 Opened 12 years ago Closed 12 years ago

Make wrappers aware of each other

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Fix (obsolete) — Splinter Review
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #423740 - Flags: review?(jst)
Blocks: 537838
blocking requested on 1.9.3 per https://bugzilla.mozilla.org/show_bug.cgi?id=537838#c1 and 2
blocking2.0: --- → ?
blocking2.0: ? → beta1
Blocks: 543856
any thoughts on when this might hit the trunk for people to play with?
Attached patch updated (obsolete) — Splinter Review
This includes fixes for a mochichrome test and session store (why are they still using evalInSandbox?).
Attachment #423740 - Attachment is obsolete: true
Attachment #426611 - Flags: review?(jst)
Attachment #423740 - Flags: review?(jst)
Attached patch updated (obsolete) — Splinter Review
Attachment #426611 - Attachment is obsolete: true
Attachment #428351 - Flags: review?(jst)
Attachment #426611 - Flags: review?(jst)
Blocks: 548416
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.
Attachment #428351 - Flags: review?(jst) → review+
Depends on: 550356
(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.
Attached patch updatedSplinter Review
Attachment #428351 - Attachment is obsolete: true
Attachment #430697 - Flags: review?(jst)
Unfortunately, interdiff doesn't work on the two patches and I don't have a good way of generating one.
Attachment #430697 - Flags: review?(jst) → review+
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.
Attached patch Additional fixSplinter Review
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).
Attachment #432180 - Flags: review?(jst)
Attachment #432180 - Flags: review?(jst) → review+
http://hg.mozilla.org/mozilla-central/rev/8dc04a747e92
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 537838
Depends on: 554448
You need to log in before you can comment on or make changes to this bug.