865961 - Root the "scope" argument to methods we use to wrap XPConnect objects in WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Root the scope argument of wrap-the-xpconnect-object helpers in WebIDL bindings.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742160 [details] [diff] [review]
Root the scope argument of wrap-the-xpconnect-object helpers in WebIDL bindings.

Needs more work.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Root the scope argument of wrap-the-xpconnect-object helpers in WebIDL bindings.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 742205 [details] [diff] [review]
Root the scope argument of wrap-the-xpconnect-object helpers in WebIDL bindings.

Review of attachment 742205 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.cpp
@@ +664,5 @@
>  
>  JSBool
>  QueryInterface(JSContext* cx, unsigned argc, JS::Value* vp)
>  {
>    JS::Value thisv = JS_THIS(cx, vp);

(Interesting; everyone else uses JS_THIS_OBJECT.

This is probably another rooting hazard, given that we use thisv at the end of the function.)

::: dom/bindings/BindingUtils.h
@@ +896,5 @@
>  // Only set allowNativeWrapper to false if you really know you need it, if in
>  // doubt use true. Setting it to false disables security wrappers.
>  bool
> +XPCOMObjectToJsval(JSContext* cx, JS::Handle<JSObject*> scope,
> +                   xpcObjectHelper &helper, const nsIID* iid,

& to the left while you're here

@@ +955,5 @@
>  
>  // Helper to make it possible to wrap directly out of an nsCOMPtr
>  template<class T>
>  inline bool
> +WrapObject(JSContext* cx, JS::Handle<JSObject*> scope, const nsCOMPtr<T> &p,

&, and three more times below

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Interesting; everyone else uses JS_THIS_OBJECT.

Which calls JS_ComputeThis, note....

This only really matters for bareword QueryInterface once Window is using WebIDL, in practice.

> This is probably another rooting hazard

Indeed.  Will fix when I do a final sweep through this stuff.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> > Interesting; everyone else uses JS_THIS_OBJECT.
> 
> Which calls JS_ComputeThis, note....

So does JS_THIS; JS_THIS_VALUE is the one that doesn't.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, right.  Anyway, CallArgs will make us revisit all that junk. ;)

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90ba502ea1fb

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90ba502ea1fb