912581 - Make JS_Add*Root APIs take Heap<T>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

To make the guidance on which GC wrapper types to use simpler, bz suggested that we make these APIs take Heap<T>s.  As I understand it, it is safe although unnecessary to post barrier things which are going to be marked as roots anyway.

This would allows us to tell people to always use Heap<T> for GC things on the heap without exception.

Terrence, what do you think of this?

Comment 1

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> To make the guidance on which GC wrapper types to use simpler, bz suggested
> that we make these APIs take Heap<T>s.  As I understand it, it is safe
> although unnecessary to post barrier things which are going to be marked as
> roots anyway.
> 
> This would allows us to tell people to always use Heap<T> for GC things on
> the heap without exception.
> 
> Terrence, what do you think of this?

That seems pretty reasonable. The only thing I am worried about is that there are somewhat valid uses of AddRoot for stack things and this would be decidedly unsafe. Of course, the static analysis in bug 885515 would prevent this usage, so I think it would be fine after that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Seems like if we want to we can keep allowing AddRoot for raw pointers too, right?

Comment 3

[Terrence Cole [:terrence]]

Yes, we could. In practice, I think it's /very/ unlikely for stack Heap<T> + AddRoot to actually be a problem, given the name and the convenience of Rooted. Let's just go ahead and do the conversion and just fix anything that crops up.

Comment 4

[Jon Coppeard (:jonco)]

This interacts slightly with global variables (used in a couple of places in the shell and jsapi-tests).  These need to be cleared before they get destroyed on exit, otherwise they will attempt to relocate themselves in the no-longer-alive store buffer.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug912581-add-root-with-heap-t

Requesting review for js changes.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

Requesting review for the browser parts.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

Requesting review for the XPConnect parts.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

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

r=me on the XPConnect bits modulo the below.

::: js/src/jsfriendapi.h
@@ +155,5 @@
>  JS_FRIEND_API(bool)
> +js_AddValueRoot(JSContext *cx, JS::Value *vp, const char *name);
> +
> +JS_FRIEND_API(void)
> +js_RemoveValueRoot(JSContext *cx, JS::Value *vp);

I think we want these to be js::Foo rather than js_Foo, right?

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2118,5 @@
>      // indirectly, regardless of in/out-ness.
>      if (type_tag == nsXPTType::T_JSVAL) {
>          // Root the value.
>          dp->val.j = JSVAL_VOID;
> +        if (!js_AddValueRoot(mCallContext, &dp->val.j, "XPCWrappedNative::CallMethod param"))

So, this is just the same behavior, but we're shunting it into a more private API where it's going to be used less-often? If so, can we name it something different, so that it's obvious?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Bobby Holley (:bholley) from comment #8)

> So, this is just the same behavior, but we're shunting it into a more
> private API where it's going to be used less-often? If so, can we name it
> something different, so that it's obvious?

That's right.  There's already js_AddObjectRoot() so I was just copying that naming, but something that says "don't use this unless you really have to" would be better.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

r=me on the bindings/ipc bits

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

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

This looks ok to me, but I wanted to ask Terrence about the naming questions. (Most of the name changes could totally be done as a separate patch, but we might want to do the js:: ones here.) Terrence, see my and bholley's comments.

::: ipc/testshell/TestShellParent.cpp
@@ +58,5 @@
>  
>  bool
>  TestShellCommandParent::RunCallback(const nsString& aResponse)
>  {
> +  NS_ENSURE_TRUE(mCallback.get() != JSVAL_NULL && mCx, false);

I totally don't know what's going on here, but would mCallback->isNull() work?

::: js/src/jsapi.h
@@ +1906,5 @@
>   * debugging if you should fail to do JS_Remove*Root(cx, &structPtr->memberObj)
>   * before freeing structPtr's memory.
>   */
>  extern JS_PUBLIC_API(bool)
> +JS_AddValueRoot(JSContext *cx, JS::Heap<JS::Value> *vp);

Now that we're C++, I do wonder if all of these should be JS::AddRoot overloads with a mandatory name argument. Then the jsfriendapi.h versions could be js::AddRawRoot. Or maybe make them regular jsapi.h JS::AddRawRoot api functions.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: bug912581-add-root-with-heap-t v2

I've made the naming changes as suggested by Bobby and Steve - the friendapi ones have become js::AddRawThingRoot and the public api ones have become JS::AddThingRoot.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8406807 [details] [diff] [review]
bug912581-add-root-with-heap-t v2

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

r=me The naming looks great.

To answer Steve's question:

The reason we have them as JS::AddFooRoot still instead of JS::AddRoot(JS::Foo *) is because the browser frequently represents gcthing pointers as a void*/JSGCTraceKind pair. In theory it is harder to call the wrong method if you have to cast and use the right name at the same time.

Ideally we'd use Value here, but JSScript* does not have a Value representation. And, of course, since all of our types are opaque outside the engine it means we can't really conveniently expose a compatible Cell*, although reinterpret_cast at api boundaries is an option. A heavier, but more convenient, alternative would be a tagged pointer; like Value in structure, but explicitly for the publicly exposed types. I'm really not sure what the best approach would be, but I'd be leery of collapsing these into C++ overrides until we have that sorted out.

Comment 14

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce496597e5eb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce496597e5eb