Last Comment Bug 776332 - Crash [@ JSFunction::toString] with --enable-root-analysis
: Crash [@ JSFunction::toString] with --enable-root-analysis
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla17
Assigned To: :Benjamin Peterson
: general
Mentors:
Depends on:
Blocks: jsfunfuzz 773746
  Show dependency treegraph
 
Reported: 2012-07-22 00:44 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-07-25 08:10 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (5.79 KB, text/plain)
2012-07-22 00:44 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fix rooting (10.76 KB, patch)
2012-07-24 10:43 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
fix rooting (11.47 KB, patch)
2012-07-24 11:58 PDT, :Benjamin Peterson
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-07-22 00:44:28 PDT
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.
Comment 1 :Benjamin Peterson 2012-07-24 10:43:28 PDT
Created attachment 645373 [details] [diff] [review]
fix rooting
Comment 2 Terrence Cole [:terrence] 2012-07-24 11:19:36 PDT
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 2012-07-24 11:26:08 PDT
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] 2012-07-24 11:30:17 PDT
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 2012-07-24 11:58:44 PDT
Created attachment 645414 [details] [diff] [review]
fix rooting

Is this more the idea?
Comment 6 Terrence Cole [:terrence] 2012-07-24 13:34:29 PDT
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 8 Ed Morley [:emorley] 2012-07-25 08:10:58 PDT
https://hg.mozilla.org/mozilla-central/rev/8e4a64b0e87f

Note You need to log in before you can comment on or make changes to this bug.