865969 - Fix remaining miscellaneous rooting bugs in WebIDL bindings

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Need to get some other patches landed first, to see what's left.

Comment 1

[Andrew McCreight [:mccr8]]

There's a few unrooted things in the generated Constructor method for JS implemented WebIDL.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Better rooting in bindings for 'object' arguments, as well as for worker interface arguments passed as JSObject*.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Better rooting in bindings for 'any' arguments.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add ways of creating Handle<Value> holding null and undefined values.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass handles to WebIDL dictionary init.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Root the id argument of WebIDL DefineInterface methods.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Fix rooting hazards in BindingUtils.cpp.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Fix rooting hazards in DOMJSProxyHandler.cpp.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 745011 [details] [diff] [review]
part 1.  Better rooting in bindings for 'object' arguments, as well as for worker interface arguments passed as JSObject*.

>+class NonNullLazyRootedObject : public Maybe<JS::RootedObject>
I would prefer Maybe<JS::Rooted<JSObject*> >
JS::RootedObject will hopefully go away, and should be currently, I've been told, used only in js/*

>+{
>+public:
>+  operator JSObject&() const
Nit, space after &, or no space in other cases. It seems like we usually have
it without space, though maybe not very consistently


>+class LazyRootedObject : public Maybe<JS::RootedObject>
JS::Rooted<JSObject*>

>+{
>+public:
>+  operator JSObject*() const
>+  {
>+    return empty() ? (JSObject*) nullptr : ref();
I think I'd prefer static_cast


>-class NonNullLazyRootedObject : public Maybe<JS::RootedObject>
Aha, this is old class. Well, JS::RootedObject should still be changed to
JS::Rooted<JSObject*>

>+        # Cast of nullptr to (JSObject*) is required for template deduction.
>+        setToNullCode = "${declName}.construct(cx, (JSObject*) nullptr);"
static_cast ?

>-  void PassOptionalObject(JSContext*, const Optional<NonNull<JSObject> >&);
>-  void PassOptionalNullableObject(JSContext*, const Optional<JSObject*>&);
>+  void PassOptionalObject(JSContext*, const Optional<NonNullLazyRootedObject>&);
>+  void PassOptionalNullableObject(JSContext*, const Optional<LazyRootedObject>&);
Hmm, optional params will need to have *Lazy*? That is somewhat ugly.
But I guess I can live with that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Fix rooting hazards in CallbackObject.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, the problem with Optional is it totally exposes the actual type.  :(  Even in the best case we'd end up with Optional<JS::Rooted<JSObject*> > and the like.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and I'll address the other comments.  But in the morning.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remaining miscellaneous rooting fixes in WebIDL bindings.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Yes, the problem with Optional is it totally exposes the actual type.  :( 
> Even in the best case we'd end up with Optional<JS::Rooted<JSObject*> > and
> the like.  :(
Well, that would be better, IMO, since JS::Rooted is what is used elsewhere, so one doesn't need
to learn a new API.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 745012 [details] [diff] [review]
part 2.  Better rooting in bindings for 'any' arguments.


Again, would prefer if one could use Optional<JS::Rooted<JS::Value> > as param, but can
live with *Lazy*


At some point we should change JS::Value and JSObject params to
JS::Handle<JS::Value> and JS::Handle<JSObject*>

Comment 16

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

Comment on attachment 745058 [details] [diff] [review]
part 9.  Remaining miscellaneous rooting fixes in WebIDL bindings.

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

Looks sensible enough.

::: dom/bindings/BindingUtils.h
@@ +669,5 @@
>    if (!value) {
>      NS_RUNTIMEABORT("Don't try to wrap null objects");
>    }
>    // We try to wrap in the compartment of the underlying object of "scope"
> +  JS::Rooted<JSObject*> obj(cx);

Can you replace

if (obj) {
  MOZ_ASSERT(tookOwnership);
}

by MOZ_ASSERT_IF(obj, tookOwnership); in this function?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 745014 [details] [diff] [review]
part 4.  Pass handles to WebIDL dictionary init.

>   bool nativeEOL = false;
>   if (aArgc > 1) {
>     if (NS_IsMainThread()) {
>       BlobPropertyBag d;
>-      if (!d.Init(aCx, aArgv[1])) {
>+      if (!d.Init(aCx, JS::Handle<JS::Value>::fromMarkedLocation(&aArgv[1]))) {
Not exactly beautiful :/

>+++ b/content/base/src/nsXMLHttpRequest.h
>@@ -166,17 +166,17 @@ public:
>   static already_AddRefed<nsXMLHttpRequest>
>   Constructor(const mozilla::dom::GlobalObject& aGlobal,
>               JSContext* aCx,
>               const nsAString& ignored,
>               ErrorResult& aRv)
>   {
>     // Pretend like someone passed null, so we can pick up the default values
>     mozilla::dom::MozXMLHttpRequestParameters params;
>-    if (!params.Init(aCx, JS::NullValue())) {
>+    if (!params.Init(aCx, JS::JSVALUE_NULLHANDLE)) {
Seriously? That look really odd. Could you file a followup to get reasonable looking null handle. Or at least discuss
with terrence about it.

Comment 18

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

Comment on attachment 745054 [details] [diff] [review]
part 7.  Fix rooting hazards in DOMJSProxyHandler.cpp.

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

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +213,5 @@
>                                          JS::AutoIdVector& props)
>  {
>    for (uint32_t i = 0; i < names.Length(); ++i) {
> +    JS::Rooted<JS::Value> v(cx);
> +    if (!xpc::NonVoidStringToJsval(cx, names[i], v.address())) {

Should take a MutableHandle at some point

Comment 19

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

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 745014 [details] [diff] [review]
> part 4.  Pass handles to WebIDL dictionary init.
> 
> >   bool nativeEOL = false;
> >   if (aArgc > 1) {
> >     if (NS_IsMainThread()) {
> >       BlobPropertyBag d;
> >-      if (!d.Init(aCx, aArgv[1])) {
> >+      if (!d.Init(aCx, JS::Handle<JS::Value>::fromMarkedLocation(&aArgv[1]))) {
> Not exactly beautiful :/

We can make it better after bug 865410, fortunately.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 745053 [details] [diff] [review]
part 6.  Fix rooting hazards in BindingUtils.cpp.


> bool
>-HasPropertyOnPrototype(JSContext* cx, JSObject* proxy, DOMProxyHandler* handler,
>-                       jsid id)
>+HasPropertyOnPrototype(JSContext* cx, JS::Handle<JSObject*> proxy,
>+                       DOMProxyHandler* handler,
>+                       JS::Handle<jsid> id)
> {
>+  JS::Rooted<JSObject*> obj(cx, proxy);
>   Maybe<JSAutoCompartment> ac;
>-  if (xpc::WrapperFactory::IsXrayWrapper(proxy)) {
>-    proxy = js::UncheckedUnwrap(proxy);
>-    ac.construct(cx, proxy);
>+  if (xpc::WrapperFactory::IsXrayWrapper(obj)) {
>+    obj = js::UncheckedUnwrap(obj);
>+    ac.construct(cx, obj);
>   }
possible extra rooting in case of !xray, but ok.

>-NativeToString(JSContext* cx, JSObject* wrapper, JSObject* object, const char* pre,
>+NativeToString(JSContext* cx, JS::Handle<JSObject*> wrapper,
>+               JS::Handle<JSObject*> obj, const char* pre,
>                const char* post, JS::Value* v)
> {
>-  JS::Rooted<JSObject*> obj(cx, object);
>-
>-  JSPropertyDescriptor toStringDesc;
>-  toStringDesc.obj = nullptr;
>-  toStringDesc.attrs = 0;
>-  toStringDesc.shortid = 0;
>-  toStringDesc.getter = nullptr;
>-  toStringDesc.setter = nullptr;
>-  toStringDesc.value = JS::UndefinedValue();
>-  if (!XrayResolveNativeProperty(cx, wrapper, obj,
>-                                 nsXPConnect::GetRuntimeInstance()->GetStringID(XPCJSRuntime::IDX_TO_STRING),
>-                                 &toStringDesc)) {
>+  JS::Rooted<JSPropertyDescriptor> toStringDesc(cx);
>+  toStringDesc.object().set(nullptr);
>+  toStringDesc.setAttributes(0);
>+  toStringDesc.setShortId(0);
>+  toStringDesc.setGetter(nullptr);
>+  toStringDesc.setSetter(nullptr);
>+  toStringDesc.value().set(JS::UndefinedValue());
>+  JS::Rooted<jsid> id(cx,
>+    nsXPConnect::GetRuntimeInstance()->GetStringID(XPCJSRuntime::IDX_TO_STRING));
>+  if (!XrayResolveNativeProperty(cx, wrapper, obj, id,
>+                                 toStringDesc.address())) {
>     return false;
>   }
I'm not familiar with JSPropertyDescriptor at all, but looks like it is rooted this way elsewhere.


>+def CreateBindingJSObject(descriptor, properties, parent):
>+    needRoot = (properties.unforgeableAttrs.hasNonChromeOnly() or
>+                properties.unforgeableAttrs.hasChromeOnly())
This could use some comment. Why rooting is needed in those cases.

> class CGResolveOwnProperty(CGAbstractMethod):
>     def __init__(self, descriptor):
>-        args = [Argument('JSContext*', 'cx'), Argument('JSObject*', 'wrapper_'),
>-                Argument('JSObject*', 'obj'), Argument('jsid', 'id_'),
>+        args = [Argument('JSContext*', 'cx'),
>+                Argument('JS::Handle<JSObject*>', 'wrapper'),
>+                Argument('JS::Handle<JSObject*>', 'obj'),
>+                Argument('JS::Handle<jsid>', 'id'),
I assume the js::RootedObject/Id I have in ResolveOwnProperty are removed somewhere.
Though, my tree might not be quite up to date.
Anyway, this just wouldn't compile if those would be still there, since
they are named wrapper and id

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 745013 [details] [diff] [review]
part 3.  Add ways of creating Handle<Value> holding null and undefined values.

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

r=me

Comment 22

[Terrence Cole [:terrence]]

Comment on attachment 745014 [details] [diff] [review]
part 4.  Pass handles to WebIDL dictionary init.

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

r=me

Out of curiosity, how much do we save by eliminating the NullValue constructors?

Comment 23

[Terrence Cole [:terrence]]

(In reply to Terrence Cole [:terrence] from comment #22)
> Out of curiosity, how much do we save by eliminating the NullValue
> constructors?

Sorry, nevermind, I thought we had recently added NullValue to match NullPtr, but I see that I was mistaken.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Well, that would be better, IMO, since JS::Rooted is what is used elsewhere

Right.  I'll need to think about how to do that; I can do it for on-stack things, with enough rewriting of codegen, but variadics and sequences and callback stuff become tricky...  I'll file a followup on it.

> At some point we should change JS::Value and JSObject params to
> JS::Handle<JS::Value> and JS::Handle<JSObject*>

Maybe.  Codegen would _really_ like to have C++ argument types and C++ return value types all look the same, because then conversion to/from JS is the same for both interfaces and callbacks.

But Handle and Rooted are pretty asymmetric.  So they're a huge pain to work with.  :(

We'll see what all that means in practice for callback codegen.

For pure-C++ callees, I think they should end up being able to use Handle arguments, yes.

> by MOZ_ASSERT_IF(obj, tookOwnership); in this function?

Done.

> Not exactly beautiful :/

No, it's not.  Moving more stuff to CallArgs will help.

> Seriously?

Per discussion on IRC just now, these will become JS::Value::NullHandle and JS::Value::UndefinedHandle.

> Should take a MutableHandle at some point

Yes, but need to update all the callers at once, so a bit of a pain.

At least now codegen _has_ a handle to pass in... but note that it's not necessarily a mutable handle!  That part will be extra fun.  ;)

> possible extra rooting in case of !xray, but ok.

Yeah, it was hard to avoid.

> This could use some comment. Why rooting is needed in those cases.

Done.

> I assume the js::RootedObject/Id I have in ResolveOwnProperty are removed
> somewhere.

Yep, right in the patch that's renaming the argument and changing it to a handle.

Comment 25

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/705880d23437
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d98854b1c001
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0fabe9f6a89
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f088bdf15d4e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7dbbff05f48
   https://hg.mozilla.org/integration/mozilla-inbound/rev/35c6ad8bb2e4
   https://hg.mozilla.org/integration/mozilla-inbound/rev/53c579eac0de
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6fb38754e1
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd740c1533fe

I'll get a followup bug filed to make the object/any situation better.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> I'll file a followup on it.

Bug 868715.

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/705880d23437
https://hg.mozilla.org/mozilla-central/rev/d98854b1c001
https://hg.mozilla.org/mozilla-central/rev/e0fabe9f6a89
https://hg.mozilla.org/mozilla-central/rev/f088bdf15d4e
https://hg.mozilla.org/mozilla-central/rev/f7dbbff05f48
https://hg.mozilla.org/mozilla-central/rev/35c6ad8bb2e4
https://hg.mozilla.org/mozilla-central/rev/53c579eac0de
https://hg.mozilla.org/mozilla-central/rev/7b6fb38754e1
https://hg.mozilla.org/mozilla-central/rev/fd740c1533fe