Closed Bug 776332 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: gkw, Assigned: Benjamin)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Attached patch fix rooting (obsolete) — Splinter Review
Assignee: general → bpeterson
Attachment #645373 - Flags: review?(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);
    ...
}
Attachment #645373 - Flags: review?(terrence)
I actually wanted to take FunctionToString out of JSFunction anyway. It's nearly a static function except that JSAPI wants to use.
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.
Attached patch fix rootingSplinter Review
Is this more the idea?
Attachment #645373 - Attachment is obsolete: true
Attachment #645414 - Flags: review?(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.
Attachment #645414 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/8e4a64b0e87f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.