776332 - Crash [@ JSFunction::toString] with --enable-root-analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

try {
    gczeal(6)
    function x() {}
    uneval(this)
} catch (e) {}

crashes js debug shell on m-c changeset defbe00ca091 without any CLI arguments with --enable-root-analysis at JSFunction::toString

Assuming related to bug 773746.

Comment 1

[:Benjamin Peterson]

Attachment: fix rooting

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 645373 [details] [diff] [review]
fix rooting

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

That's not quite what I had in mind :-).  First, Root as close to the top of the stack as possible -- e.g. in JS_DecompileFunction.  Secondly, we still want to use the class to group functions -- i.e. we don't want to just go to all flat static functions.  I had in mind something more like:

JS_PUBLIC_API(JSString *)
JS_DecompileFunction(JSContext *cx, JSFunction *funArg, unsigned indent)
{
    RootedFunction fun(cx, funArg);
    ...
    return Function::toString(fun, cx, false, !(indent & JS_DONT_PRETTY_PRINT));
}

class JSFunction {
    ...
    static JSString *toString(HandleFunction self, JSContext *cx, bool bodyOnly, bool pretty);
    ...
}

Comment 3

[:Benjamin Peterson]

I actually wanted to take FunctionToString out of JSFunction anyway. It's nearly a static function except that JSAPI wants to use.

Comment 4

[Terrence Cole [:terrence]]

Ah, I can't really speak to whether toString belongs in JSFunction or out of it, so in that case it's fine to move it out.  FunctionToString should still take |fun| as a HandleFunction and root it in JS_DecompileFunction, however.

Comment 5

[:Benjamin Peterson]

Attachment: fix rooting

Is this more the idea?

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 645414 [details] [diff] [review]
fix rooting

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

Perfect.

::: js/src/jsapi.cpp
@@ +5348,5 @@
>      return script->sourceData(cx);
>  }
>  
>  JS_PUBLIC_API(JSString *)
> +JS_DecompileFunction(JSContext *cx, JSFunction *fun_, unsigned indent)

The current style for this is |funArg|. The reason most of them are trailing-_ at the moment is that we only decided on the style recently and these names will all be going away when we handlify the interface in the near future.  You don't have to change this, just pointing it out for future reference.

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a64b0e87f

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e4a64b0e87f