738479 - Debugger.Script objects whose referents are non-compile-and-go are crashy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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]]

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]]

Attachment: v1

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Forgot to hg add the test .js file.

Comment 4

[Jim Blandy :jimb]

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]]

Attachment: 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 7

[Jim Blandy :jimb]

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 (he/him) [:past] (please ni?)]

Carrying over the K9O requirement from duplicate bug 746973.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v4

Comment 10

[Jim Blandy :jimb]

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]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ea1e180ed52c