Closed Bug 865961 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 865969
Comment on attachment 742160 [details] [diff] [review]
Root the scope argument of wrap-the-xpconnect-object helpers in WebIDL bindings.

Needs more work.
Attachment #742160 - Attachment is obsolete: true
Attachment #742160 - Flags: review?(Ms2ger)
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
Attachment #742205 - Flags: review?(Ms2ger) → review+
> 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.
(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.
Ah, right.  Anyway, CallArgs will make us revisit all that junk. ;)
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/90ba502ea1fb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.