Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 444754 [details] [diff] [review]
patch

The 'void *' parameter of JS_AddRoot is a classic source of mistakes.  It's also a serious problem for fat values (bug 549143), since jsvals and gc-things will have different sizes and no distinguishing prefix.

This patch fixes both problems by splitting JS_(Add[Named]|Remove)Root into typed versions:

JSBool JS_AddValueRoot(JSContext *cx, jsval *vp);
JSBool JS_AddStringRoot(JSContext *cx, JSString **rp);
JSBool JS_AddObjectRoot(JSContext *cx, JSObject **rp);
JSBool JS_AddDoubleRoot(JSContext *cx, jsdouble **rp);
JSBool JS_AddGCThingRoot(JSContext *cx, void **rp);
JSBool JS_AddNamedValueRoot(JSContext *cx, jsval *vp, const char *name);
JSBool JS_AddNamedStringRoot(JSContext *cx, JSString **rp, const char *name);
JSBool JS_AddNamedObjectRoot(JSContext *cx, JSObject **rp, const char *name);
JSBool JS_AddNamedDoubleRoot(JSContext *cx, jsdouble **rp, const char *name);
JSBool JS_AddNamedGCThingRoot(JSContext *cx, void **rp, const char *name);
JSBool JS_AddNamedValueRootRT(JSRuntime *rt, jsval *vp, const char *name);
JSBool JS_AddNamedStringRootRT(JSRuntime *rt, JSString **rp, const char *name);
JSBool JS_AddNamedObjectRootRT(JSRuntime *rt, JSObject **rp, const char *name);
JSBool JS_AddNamedDoubleRootRT(JSRuntime *rt, jsdouble **rp, const char *name);
JSBool JS_AddNamedGCThingRootRT(JSRuntime *rt, void **rp, const char *name);
JSBool JS_RemoveValueRoot(JSContext *cx, jsval *vp);
JSBool JS_RemoveStringRoot(JSContext *cx, JSString **rp);
JSBool JS_RemoveObjectRoot(JSContext *cx, JSObject **rp);
JSBool JS_RemoveDoubleRoot(JSContext *cx, jsdouble **rp);
JSBool JS_RemoveGCThingRoot(JSContext *cx, void **rp);
JSBool JS_RemoveValueRootRT(JSRuntime *rt, jsval *vp);
JSBool JS_RemoveStringRootRT(JSRuntime *rt, JSString **rp);
JSBool JS_RemoveObjectRootRT(JSRuntime *rt, JSObject **rp);
JSBool JS_RemoveDoubleRootRT(JSRuntime *rt, jsdouble **rp);
JSBool JS_RemoveGCThingRootRT(JSRuntime *rt, void **rp);

Most uses in mozilla are easily updated.  The only annoying part is when wrappers were made for JS_AddRoot/JS_RemoveRoot that themselves take a void*.

Igor, let me know if I should assign an additional reviewer for the non-js changes.
Attachment #444754 - Flags: review?(igor)

Comment 1

7 years ago
(In reply to comment #0)
> Created an attachment (id=444754) [details]
> patch

The patch must keep both JS_AddRoot and JS_AddNamedRoot if we want compatibility. Also we do not need the RT version of the typed functions.

In the DEBUG builds JS_AddNamedRoot and JS_AddNamedRootRT should print a deprecation warning to stderr the first time they are used.  

> Igor, let me know if I should assign an additional reviewer for the non-js
> changes.

No, since that are straightforward changes AFAICS on the first glance. But please split patch in two: js/src/* changes and everything else.

Updated

7 years ago
Attachment #444754 - Flags: review?(igor)
(Assignee)

Comment 2

7 years ago
(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=444754) [details] [details]
> > patch
> 
> The patch must keep both JS_AddRoot and JS_AddNamedRoot if we want
> compatibility.

We had discussed breaking compatibility on this; the fat value patch requires it: it cannot distinguish a gc-thing from a jsval.  The idea behind the patch is move away from the scary broken API earlier than later.

> Also we do not need the RT version of the typed functions.

The RT versions are used in mozilla.  It makes sense that one would want to add a root to the runtime, without having a particular context in hand.
Right, we have to break. Do it now, the old API is just a trap. Please notify the js-engine group.

The rt-parameterized versions still make sense even with GC compartments, indeed.

/be

Comment 4

7 years ago
Can we please have a C precompiler test to use, so that we can write code that works with spidermonkey releases both before and after this lands?
(Assignee)

Comment 5

7 years ago
Created attachment 445236 [details] [diff] [review]
changes to js/src

Split the patch per comment 1.  Added #define JS_HAS_TYPED_ROOTING_API in jsapi.h per comment 4 (couldn't think of a better name, plus, Australian programmers must be used to this one by now).
Attachment #444754 - Attachment is obsolete: true
Attachment #445236 - Flags: review?(igor)
(Assignee)

Comment 6

7 years ago
Created attachment 445237 [details] [diff] [review]
mozilla changes
Attachment #445237 - Flags: review?(igor)
(In reply to comment #5)
> Added #define JS_HAS_TYPED_ROOTING_API in
> jsapi.h per comment 4 (couldn't think of a better name, plus, Australian
> programmers must be used to this one by now).

"root", yes, but "rooting" still warrants a giggle, especially when its in all-caps.
(Assignee)

Comment 8

7 years ago
JS_HAS_TYPED_ROOT_API?

Comment 9

7 years ago
(In reply to comment #2)
> The RT versions are used in mozilla.  It makes sense that one would want to add
> a root to the runtime, without having a particular context in hand.

The problem with the RT version is that they are explicitly allowed to be called outside of the request. We should really not proliferate them and explicitly require that any rooting happens explicitly inside a request with the exception that RemooveRoot can also happens during the finalization.
(Assignee)

Comment 10

7 years ago
(In reply to comment #9)
> The problem with the RT version is that they are explicitly allowed to be
> called outside of the request. We should really not proliferate them and
> explicitly require that any rooting happens explicitly inside a request with
> the exception that RemooveRoot can also happens during the finalization.

That sounds like a separate bug.  This patch doesn't proliferate, it just maintains.

Comment 11

7 years ago
(In reply to comment #10)
> That sounds like a separate bug.  This patch doesn't proliferate, it just
> maintains.

It proliferates in sense of adding new functions RT functions as a replacement for single older one. If the users would be forced to update their code in any case we should also try to remove cases of the problematic usage. Thus I would prefer to remove the RT functions.

Granted, in few cases of their usage in mozilla codebase they are convenient as there is no trivially accessible JSContext instance to use. But in those cases the usage of JS_AddRoot is rather suboptimal and should be really replaced by one of the autorooters. If converting those case to the cx version of typed AddRoot functions is hard to do quickly, then at least we should just add the RT functions that are necessary to support mozilla's usage as JS_FRIEND_API to jsgc.h. We can remove that bad usage later in separated bug.

Comment 12

7 years ago
Luke: what is the status here? As I wrote above, we should try to avoid the RT functions unless it would be absolutely necessary for compatibility. Otherwise the patch is absolutely OK.
(Assignee)

Comment 13

7 years ago
Taking out the RT calls isn't a very quick fix so it keeps getting preempted by other things, primarily fatvals.

Comment 14

7 years ago
(In reply to comment #13)
> Taking out the RT calls isn't a very quick fix so it keeps getting preempted by
> other things, primarily fatvals.

If it would help, just keep as friends those that necessary to build the browser. I will do in a followup their replacement with non-RT versions. And another observation is that in view of fatvals AddNamedDoubleRoot would be soon unnecessary.
(Assignee)

Comment 15

7 years ago
Created attachment 449350 [details] [diff] [review]
changes to js/src

Now with the RT versions as friend APIs.
Attachment #445236 - Attachment is obsolete: true
Attachment #449350 - Flags: review?(igor)
Attachment #445236 - Flags: review?(igor)
(Assignee)

Comment 16

7 years ago
Created attachment 449351 [details] [diff] [review]
mozilla changes
Attachment #445237 - Attachment is obsolete: true
Attachment #449351 - Flags: review?(igor)
Attachment #445237 - Flags: review?(igor)
(Assignee)

Comment 17

7 years ago
Oops, I put the JS_FRIEND-fication changes in the mozilla-changes patch instead of the js/src-changes patch.

Comment 18

7 years ago
Comment on attachment 449350 [details] [diff] [review]
changes to js/src

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -1759,47 +1759,120 @@ JS_NewDoubleValue(JSContext *cx, jsdoubl
>+JS_PUBLIC_API(JSBool)
>+JS_AddStringRoot(JSContext *cx, JSString **rp)
>+{
>+    CHECK_REQUEST(cx);
>+    return js_AddGCThingRoot(cx, (void **)rp, NULL);
>+}

Let's use a union type as a parameter for js_AddRot and js_RemoveRoot to prevent aliasing warnings from GCC, with a usage like:

JS_AddStringRoot(JSContext *cx, JSString **rp)
{
    CHECK_REQUEST(cx);
    TypedRootPointer u;
    u.strp = rp;
    return js_AddGCThingRoot(cx, u, NULL);
}

This makes sure that GCC would not complain about aliasing warnings.

>+/*
>+ * This symbol may be used by embedders to detect the change from the old
>+ * JS_AddRoot(JSContext *, void *) APIs to the new ones above.
>+ */
>+#define JS_HAS_TYPED_ROOTING_API

Nit: rename this into JS_TYPED_ROOTING_API. JS_HAS is used for features that can be switched off during compilation. This is not the case here.

r+ with this fixed.
Attachment #449350 - Flags: review?(igor) → review+

Updated

7 years ago
Attachment #449351 - Flags: review?(igor) → review+
(Assignee)

Comment 19

7 years ago
I'd rather not add the union.  There isn't a strict-aliasing hazard here to warn about, or even the appearance of one, and even if there was, the union wouldn't fix it.  The proposed union is for the *pointer* to the GC thing.  To actually improve anything, you would need a union U over JSString*, JSObject*, and double* and have js_AddRoot take a U*.  But then callers would need to cast from X** or void* to U* and so nothing is improved, but everything is a bit more verbose.

Comment 20

7 years ago
(In reply to comment #19)
> I'd rather not add the union. There isn't a strict-aliasing hazard here to
> warn about, or even the appearance of one, and even if there was, the union
> wouldn't fix it.  The proposed union is for the *pointer* to the GC thing.  To
> actually improve anything, you would need a union U over JSString*, JSObject*,
> and double* and have js_AddRoot take a U*.  But then callers would need to cast
> from X** or void* to U* and so nothing is improved, but everything is a bit
> more verbose.

Right, so just r+
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/tracemonkey/rev/2fc2a12a4565
Whiteboard: fixed-in-tracemonkey

Comment 22

7 years ago
http://hg.mozilla.org/mozilla-central/rev/2fc2a12a4565
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 578811
You need to log in before you can comment on or make changes to this bug.