805315 - Use Return<T> in a few places

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I have some patches to add Return<T> in a few places.  I'm starting off small since I'm new at this.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use Return<T> in jsiter.cpp.

This patch wraps some return values in jsiter.cpp.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use Return<T> in jswrapper.cpp.

This patch wraps some return values in jswrapper.cpp.

It doesn't wrap the return value of TransparentObjectWrapper because it's an
instance of JSWrapObjectCallback, which is exposed in jsapi.h :(

This passes jit-tests and jstests.  I'll do a try server run as well.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use Return<T> in jsiter.cpp.

Try server found an assertion failure, which this version fixes.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 674997 [details] [diff] [review]
(part 1) - Use Return<T> in jsiter.cpp.

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

Looks good.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 674960 [details] [diff] [review]
(part 2) - Use Return<T> in jswrapper.cpp.

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

Okay.

::: js/src/jswrapper.cpp
@@ +39,5 @@
>      return &sWrapperFamily;
>  }
>  
> +Return<JSObject*>
> +Wrapper::New(JSContext *cx, JSObject *obj, JSObject *proto, JSObject *parent, Wrapper *handler)

Should these be Handle or Raw?

@@ +452,4 @@
>  /* Compartments. */
>  
>  extern JSObject *
> +js::TransparentObjectWrapper(JSContext *cx, JSObject *obj, JSObject *wrappedProto, JSObject *parent,

I think make these Raw as well.

@@ +846,4 @@
>  
>      RootedObject proto(cx);
>      {
> +        RootedObject wrapped(cx, wrappedObject(proxy).unsafeGet());

No need for unsafeGet here.

@@ +1240,5 @@
>      // Recompute all the wrappers in the list.
>      for (Value *begin = toRecompute.begin(), *end = toRecompute.end(); begin != end; ++begin)
>      {
> +        RootedObject wrapper(cx, &begin->toObject());
> +        RootedObject wrapped(cx, Wrapper::wrappedObject(wrapper).unsafeGet());

No need for unsafeGet here.

::: js/src/jswrapper.h
@@ +366,1 @@
>  NewDeadProxyObject(JSContext *cx, JSObject *parent);

Maybe make this Handle or Raw if it isn't API?

::: js/src/shell/js.cpp
@@ +3280,5 @@
>      RootedObject proto(cx);
>      if (!JSObject::getProto(cx, obj, &proto))
>          return false;
> +    RawObject wrapped =
> +        Wrapper::New(cx, obj, proto, &obj->global(), &DirectWrapper::singleton).unsafeGet();

Please add an AssertCanGC() to the top of this function and any others that are not currently marked.

Comment 6

[Nicholas Nethercote [inactive]]

> > +Return<JSObject*>
> > +Wrapper::New(JSContext *cx, JSObject *obj, JSObject *proto, JSObject *parent, Wrapper *handler)
> 
> Should these be Handle or Raw?

Handle, but it's a friend API function so I left it unchanged.


> >  extern JSObject *
> > +js::TransparentObjectWrapper(JSContext *cx, JSObject *obj, JSObject *wrappedProto, JSObject *parent,
> 
> I think make these Raw as well.

It's also exposed to jsapi.h;  see comment 2.


> ::: js/src/shell/js.cpp
> @@ +3280,5 @@
> >      RootedObject proto(cx);
> >      if (!JSObject::getProto(cx, obj, &proto))
> >          return false;
> > +    RawObject wrapped =
> > +        Wrapper::New(cx, obj, proto, &obj->global(), &DirectWrapper::singleton).unsafeGet();
> 
> Please add an AssertCanGC() to the top of this function and any others that
> are not currently marked.

Do you mean just in shell/js.cpp, or everywhere?

Comment 7

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> > > +Return<JSObject*>
> > > +Wrapper::New(JSContext *cx, JSObject *obj, JSObject *proto, JSObject *parent, Wrapper *handler)
> > 
> > Should these be Handle or Raw?
> 
> Handle, but it's a friend API function so I left it unchanged.

Ah, right, I had forgotten that we already have C++ interfaces.
 
> > ::: js/src/shell/js.cpp
> > @@ +3280,5 @@
> > >      RootedObject proto(cx);
> > >      if (!JSObject::getProto(cx, obj, &proto))
> > >          return false;
> > > +    RawObject wrapped =
> > > +        Wrapper::New(cx, obj, proto, &obj->global(), &DirectWrapper::singleton).unsafeGet();
> > 
> > Please add an AssertCanGC() to the top of this function and any others that
> > are not currently marked.
> 
> Do you mean just in shell/js.cpp, or everywhere?

Everywhere that it makes sense.  I'm thinking our general rule should look like: any function that does not unconditionally call a New function or is only property accesses.

Comment 8

[Nicholas Nethercote [inactive]]

> I'm thinking our general rule should look
> like: any function that does not unconditionally call a New function or is
> only property accesses.

To paraphrase:
- We should add AssertCanGC to functions that conditionally call a New function (e.g. js_NewGCThing).
- We shouldn't add AssertNoGC to functions that are very simple, e.g. only do property accesses.

Comment 9

[Terrence Cole [:terrence]]

After a couple months of trying, we're ripping out Return<T>, Unrooted<T>, and our other attempts to make the platform help constrain/elucidate GC timing.