Fix remaining miscellaneous rooting bugs in WebIDL bindings

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 days ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla23
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

21.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.19 KB, patch
terrence
: review+
Details | Diff | Splinter Review
22.32 KB, patch
smaug
: review+
terrence
: review+
Details | Diff | Splinter Review
3.29 KB, patch
smaug
: review+
Details | Diff | Splinter Review
43.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.36 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
4.95 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
10.15 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
Need to get some other patches landed first, to see what's left.
There's a few unrooted things in the generated Constructor method for JS implemented WebIDL.
Depends on: 865785
Depends on: 865975
Attachment #745014 - Flags: review?(terrence)
Attachment #745014 - Flags: review?(bugs)
Attachment #745011 - Flags: review?(bugs) → review?(Ms2ger)
Attachment #745012 - Flags: review?(bugs) → review?(Ms2ger)
Attachment #745014 - Flags: review?(bugs) → review?(Ms2ger)
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.
Attachment #745011 - Flags: review?(Ms2ger) → review+
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.  :(
Oh, and I'll address the other comments.  But in the morning.  ;)
(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 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*>
Attachment #745012 - Flags: review?(Ms2ger) → review+
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?
Attachment #745058 - Flags: review?(Ms2ger) → review+
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.
Attachment #745014 - Flags: review?(Ms2ger) → review+
Attachment #745057 - Flags: review?(Ms2ger) → review+
Attachment #745044 - Flags: review?(Ms2ger) → review+
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
Attachment #745054 - Flags: review?(Ms2ger) → review+
(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 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
Attachment #745053 - Flags: review?(Ms2ger) → review+
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
Attachment #745013 - Flags: review?(terrence) → review+
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?
Attachment #745014 - Flags: review?(terrence) → review+
(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.
> 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.
> I'll file a followup on it.

Bug 868715.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.