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

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: Benjamin)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla17
x86_64
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 644733 [details]
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.
(Assignee)

Comment 1

5 years ago
Created attachment 645373 [details] [diff] [review]
fix rooting
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)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 645414 [details] [diff] [review]
fix rooting

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+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a64b0e87f
https://hg.mozilla.org/mozilla-central/rev/8e4a64b0e87f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.