If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use Return<T> in a few places

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 674946 [details] [diff] [review]
(part 1) - Use Return<T> in jsiter.cpp.

This patch wraps some return values in jsiter.cpp.
Attachment #674946 - Flags: review?(terrence)
(Assignee)

Comment 2

5 years ago
Created attachment 674960 [details] [diff] [review]
(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.
Attachment #674960 - Flags: review?(terrence)
(Assignee)

Comment 3

5 years ago
Created attachment 674997 [details] [diff] [review]
(part 1) - Use Return<T> in jsiter.cpp.

Try server found an assertion failure, which this version fixes.
Attachment #674997 - Flags: review?(terrence)
(Assignee)

Updated

5 years ago
Attachment #674946 - Attachment is obsolete: true
Attachment #674946 - Flags: review?(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.
Attachment #674997 - Flags: review?(terrence) → review+
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.
Attachment #674960 - Flags: review?(terrence) → review+
(Assignee)

Comment 6

5 years ago
> > +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?
(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.
(Assignee)

Comment 8

5 years ago
> 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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.