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]
:
: 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 | Splinter Review
v2 (8.13 KB, patch)
2012-04-23 04:05 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review
v3 (9.58 KB, patch)
2012-05-02 14:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v4 (9.56 KB, patch)
2012-05-03 13:50 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review

Description User image 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 User image 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 User image Jason Orendorff [:jorendorff] 2012-04-19 11:25:05 PDT
Created attachment 616660 [details] [diff] [review]
v1
Comment 3 User image 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 User image 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 User image 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 User image Jim Blandy :jimb 2012-05-02 14:55:40 PDT
*** Bug 746973 has been marked as a duplicate of this bug. ***
Comment 7 User image 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 User image Panos Astithas [:past] 2012-05-02 23:09:37 PDT
Carrying over the K9O requirement from duplicate bug 746973.
Comment 9 User image Jason Orendorff [:jorendorff] 2012-05-03 13:50:17 PDT
Created attachment 620838 [details] [diff] [review]
v4
Comment 10 User image 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 User image 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
Comment 12 User image :Ms2ger (⌚ UTC+1/+2) 2012-05-05 03:38:44 PDT
https://hg.mozilla.org/mozilla-central/rev/ea1e180ed52c

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