Last Comment Bug 738479 - Debugger.Script objects whose referents are non-compile-and-go are crashy
: Debugger.Script objects whose referents are non-compile-and-go are crashy
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 746973 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-22 15:11 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-05-05 03:38 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
v1 (7.33 KB, patch)
2012-04-19 11:25 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v2 (8.13 KB, patch)
2012-04-23 04:05 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review
v3 (9.58 KB, patch)
2012-05-02 14:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v4 (9.56 KB, patch)
2012-05-03 13:50 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2012-03-22 15:11:29 PDT
They are crashy because they have no global object. ScriptGlobal asserts. Trying to enter the script's compartment asserts. Et cetera.
Comment 1 Jason Orendorff [:jorendorff] 2012-03-29 11:20:17 PDT
In Rob's user repo, I landed a patch that fixes this; but obviously we need a way to test non-compile-and-go code in unit tests, if we're going to have such a thing.

https://hg.mozilla.org/users/rcampbell_mozilla.com/remote-debug/rev/c12e4bc4e8ab
Comment 2 Jason Orendorff [:jorendorff] 2012-04-19 11:25:05 PDT
Created attachment 616660 [details] [diff] [review]
v1
Comment 3 Jason Orendorff [:jorendorff] 2012-04-23 04:05:13 PDT
Created attachment 617444 [details] [diff] [review]
v2

Forgot to hg add the test .js file.
Comment 4 Jim Blandy :jimb 2012-05-02 13:27:23 PDT
Comment on attachment 617444 [details] [diff] [review]
v2

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

r=me, with the JSOPTION_COMPILE_N_GO addressed. If you want to re-review, I'll do my best to be prompt.

::: js/src/shell/js.cpp
@@ +849,5 @@
>      }
>  
> +    // Separate compile and execute steps for the benefit of EvaluteNonCompileAndGo, below.
> +    // JS_EvaluateUCScript is shorter, but it always enables the compile-and-go option.
> +    JSScript *script = JS_CompileUCScript(cx, thisobj, codeChars, codeLength, "@evaluate", 1);

Starting at line 1 seems better, yeah.

@@ +860,5 @@
> +EvaluateNonCompileAndGo(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    uint32_t saved = JS_GetOptions(cx);
> +    JS_SetOptions(cx, saved & ~JSOPTION_COMPILE_N_GO);
> +    bool ok = Evaluate(cx, argc, vp);

This approach means that JSSOPTION_COMPILE_N_GO is cleared for not just the immediate call to Evaluate, but for all nested calls, including any nested calls to 'evaluate'. I would expect that what we really want is to only have the option cleared for the compilation, but not the execution.
Comment 5 Jason Orendorff [:jorendorff] 2012-05-02 14:41:45 PDT
Created attachment 620474 [details] [diff] [review]
v3

v2 unintentionally changed the behavior of evaluate(). The pre-existing evaluate() uses JS_EvaluateScript which forces compile-and-go mode on. v3 retains that behavior (v2 didn't force it either way, and the results were therefore unpredictable).
Comment 6 Jim Blandy :jimb 2012-05-02 14:55:40 PDT
*** Bug 746973 has been marked as a duplicate of this bug. ***
Comment 7 Jim Blandy :jimb 2012-05-02 15:10:36 PDT
Comment on attachment 620474 [details] [diff] [review]
v3

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

::: js/src/shell/js.cpp
@@ +857,5 @@
> +
> +        JSScript *script = JS_CompileUCScript(cx, thisobj, codeChars, codeLength, "@evaluate", 1);
> +        bool ok = script && JS_ExecuteScript(cx, thisobj, script, vp);
> +
> +        JS_SetOptions(cx, saved);

This wants to be before the JS_ExecuteScript, right?
Comment 8 Panos Astithas [:past] 2012-05-02 23:09:37 PDT
Carrying over the K9O requirement from duplicate bug 746973.
Comment 9 Jason Orendorff [:jorendorff] 2012-05-03 13:50:17 PDT
Created attachment 620838 [details] [diff] [review]
v4
Comment 10 Jim Blandy :jimb 2012-05-03 16:53:17 PDT
Comment on attachment 620838 [details] [diff] [review]
v4

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

::: js/src/jit-test/tests/debug/breakpoint-09.js
@@ +1,3 @@
> +// Setting a breakpoint in an eval script that is not on the stack. Bug 746973.
> +// We don't assert that the breakpoint actually hits because that depends on
> +// the eval cache, an implementation detail.

How about asserting that the breakpoint hits the second time g.f is called iff the onNewScript hook doesn't fire?
Comment 11 Jim Blandy :jimb 2012-05-03 22:37:59 PDT
Comment on attachment 620838 [details] [diff] [review]
v4

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

::: js/src/vm/Debugger.cpp
@@ +2862,5 @@
>      if (!handler)
>          return false;
>  
>      jsbytecode *pc = script->code + offset;
> +    GlobalObject *scriptGlobal = script->getGlobalObjectOrNull();

By the way, shouldn't this decl continue to be a RootedVar? It certainly looks like the GC guys are trying to get everything marked up, as here:

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=745742&attachment=619232

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