Closed Bug 1146333 Opened 6 years ago Closed 6 years ago

Stop abusing WrapNativeParent in WrapCallThisObject/WrapCallThisValue


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

Not set



Tracking Status
firefox40 --- fixed


(Reporter: bzbarsky, Assigned: bzbarsky)


(Blocks 1 open bug)



(2 files)

Once bug 1146234 lands (or in lieu of it, as preferred), we should consider doing the following:

1) Moving WrapCallThisObject/WrapCallThisValue to a different header that
   includes CallbackObject.h (because we don't want to include it in
2) Make it use ToJSValue instead of WrapNativeParent.

We'd need to add a ToJSValue specialization for Rooted<JSObject*>, but that should be easy enough.  And we could remove most of the special-casing in WrapCallThisObject/Value.  Specifically, using ToJSValue would automatically handle the case of the thisval being a callback itself correctly, and automatically handle the Value/Object overloads.  In fact, we may be able to eliminate WrapCallThisObject/Value altogether and just use ToJSValue here.
Assignee: nobody → bzbarsky
Comment on attachment 8584914 [details] [diff] [review]
Get rid of WrapCallThisValue and just use ToJSValue, now that we have it

Review of attachment 8584914 [details] [diff] [review]:

Attachment #8584914 - Flags: review?(peterv) → review+
Bug 1144397, bug 1146333, and bug 1148973 backed out for widespread test bustage:
(the Linux opt build failures were from glandium's prior push, the rest is bz)
Flags: needinfo?(bzbarsky)
Yeah, I thought I'd run this patch through try but apparently not.

Found two problems so far in the test failures:

1)  There was no Rooted<JSObject*> specialization for ToJSValue, and the failure mode is that the type gets treated as a boolean (ouch).  This has been a long-standing footgun, and I propose to fix it so it stops hitting us.  Plus, of course, adding the Rooted<JSObject*> specialization.

2)  The nsISupports wrapping in ToJSValue was mainthread-only.  But event handlers have an nsISupports mTarget that they try to use as the this.  So we need to change the nsISupports bits to take a codepath that works on workers as well.
Flags: needinfo?(bzbarsky)
Attachment #8586140 - Flags: review?(peterv)
Comment on attachment 8586140 [details] [diff] [review]
This seems to fix the orange

Review of attachment 8586140 [details] [diff] [review]:

Sorry I missed the JS::Rooted<JSObject*> thing last time.

::: dom/bindings/ToJSValue.h
@@ +193,5 @@
>  {
>    // Make sure we're called in a compartment
>    MOZ_ASSERT(JS::CurrentGlobalOrNull(aCx));
> +  qsObjectHelper helper(ToSupports(&aArgument), nullptr);

I think that technically you don't need the ToSupports.
Attachment #8586140 - Flags: review?(peterv) → review+
> I think that technically you don't need the ToSupports.

I think I do if T multiply-inherits from nsISupports.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.