Last Comment Bug 776332 - Crash [@ JSFunction::toString] with --enable-root-analysis
: Crash [@ JSFunction::toString] with --enable-root-analysis
: 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
: Jason Orendorff [:jorendorff]
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: ---

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 User image Gary Kwong [:gkw] [:nth10sd] 2012-07-22 00:44:28 PDT
Created attachment 644733 [details]

try {
    function x() {}
} 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 User image :Benjamin Peterson 2012-07-24 10:43:28 PDT
Created attachment 645373 [details] [diff] [review]
fix rooting
Comment 2 User image 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_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 User image :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 User image 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 User image :Benjamin Peterson 2012-07-24 11:58:44 PDT
Created attachment 645414 [details] [diff] [review]
fix rooting

Is this more the idea?
Comment 6 User image 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]:


::: 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 User image Ed Morley [:emorley] 2012-07-25 08:10:58 PDT

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