562991 - JS_SetTrap roots the closure void*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

Unlike all other APIs that set a JSTrapHandler, JS_SetTrap roots a non-null closure argument by converting it to a jsval and passing that to js_CallValueTracerIfGCThing.  This means that there is a subtle (undocumented) restriction on what void*s can be passed by the caller. E.g., jsd has to do:

 JS_SetTrap(..., (void *)PRIVATE_TO_JSVAL(p))

so that p is not misunderstood as a gc thing.

The patch makes things simpler.  If r+, I'll post to the newsgroup about the change.  I suspect for embeders it mainly an annoyance removed (MikeM had to trip and discover the same hack as jsd), rather than a feature.  If anyone really wanted the closure removed, it is trivial to add a JS_SetTrapWithRootedValue(..., closure, v) with precondition 'JSVAL_TO_GCTHING(v) == closure' that roots v.

The main reason I file is that this void* <--> jsval stuff is trouble for bug 549143.

Comment 1

[Luke Wagner [:luke]]

Attachment: patch2

I totally forgot about the one use of the rooting done by JS_SetTrap in shell/js.cpp.  Here, it is quite useful, and it really seems possible for an embedder to have the same problem.  So instead of removing the rooting, this patch changes the signature of JS_SetTrap to take a jsval, which it marks.  JS_SetTrap shares the JSTrapHandler typedef with some other void*-no-root hooks, so the patch has to split these off.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 442866 [details] [diff] [review]
patch2

> static JSTrapStatus
> EmptyTrapHandler(JSContext *cx, JSScript *script, jsbytecode *pc, jsval *rval,
>-                 void *closure)
>+                 jsval closure)
> {
>     JS_GC(cx);
>-    ++emptyTrapCallCount;
>+    if (JSVAL_IS_STRING(closure))
>+        ++emptyTrapCallCount;

What's this change needed for? I can't see it.

r=me anyway, just hoping for clarity, or status quo ante, on this point.

/be

Comment 3

[Luke Wagner [:luke]]

Oh, it just makes the test fail if the closure passed through isn't a string, which it should be.

Comment 4

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/3e73a0559b82

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3e73a0559b82