Make JS_Add*Root APIs take Heap<T>

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(terrence)
Assignee

Updated

6 years ago
Seems like if we want to we can keep allowing AddRoot for raw pointers too, right?
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.
Assignee

Comment 4

6 years ago
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.
Blocks: 773686
No longer blocks: ExactRootingBrowser
Assignee

Updated

5 years ago
Depends on: 993413
Assignee

Comment 5

5 years ago
Requesting review for js changes.
Attachment #8406221 - Flags: review?(terrence)
Assignee

Updated

5 years ago
Attachment #8406221 - Flags: review?(terrence) → review?(sphink)
Assignee

Comment 6

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

Requesting review for the browser parts.
Attachment #8406221 - Flags: review?(bzbarsky)
Assignee

Comment 7

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

Requesting review for the XPConnect parts.
Attachment #8406221 - Flags: review?(bobbyholley)
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?
Attachment #8406221 - Flags: review?(bobbyholley) → review+
Assignee

Comment 9

5 years ago
(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 on attachment 8406221 [details] [diff] [review]
bug912581-add-root-with-heap-t

r=me on the bindings/ipc bits
Attachment #8406221 - Flags: review?(bzbarsky) → review+
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.
Attachment #8406221 - Flags: review?(sphink)
Attachment #8406221 - Flags: review+
Attachment #8406221 - Flags: feedback?(terrence)
Assignee

Comment 12

5 years ago
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.
Assignee: general → jcoppeard
Attachment #8406221 - Attachment is obsolete: true
Attachment #8406221 - Flags: feedback?(terrence)
Attachment #8406807 - Flags: review?(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.
Attachment #8406807 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/ce496597e5eb
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.