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

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla15
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

(Whiteboard: [chrome-debug])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
They are crashy because they have no global object. ScriptGlobal asserts. Trying to enter the script's compartment asserts. Et cetera.
(Assignee)

Comment 1

6 years ago
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

Updated

6 years ago
Whiteboard: [chrome-debug]
(Assignee)

Comment 2

5 years ago
Created attachment 616660 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #616660 - Flags: review?(jimb)
(Assignee)

Comment 3

5 years ago
Created attachment 617444 [details] [diff] [review]
v2

Forgot to hg add the test .js file.
Attachment #616660 - Attachment is obsolete: true
Attachment #616660 - Flags: review?(jimb)
Attachment #617444 - Flags: review?(jimb)

Comment 4

5 years ago
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.
Attachment #617444 - Flags: review?(jimb) → review+
(Assignee)

Comment 5

5 years ago
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).
Attachment #617444 - Attachment is obsolete: true
Attachment #620474 - Flags: review?(jimb)

Updated

5 years ago
Duplicate of this bug: 746973

Comment 7

5 years ago
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?

Updated

5 years ago
Attachment #620474 - Flags: review?(jimb)
Carrying over the K9O requirement from duplicate bug 746973.
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 9

5 years ago
Created attachment 620838 [details] [diff] [review]
v4
Attachment #620474 - Attachment is obsolete: true
Attachment #620838 - Flags: review?(jimb)

Comment 10

5 years ago
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?
Attachment #620838 - Flags: review?(jimb) → review+

Comment 11

5 years ago
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

Updated

5 years ago
blocking-kilimanjaro: ? → +
https://hg.mozilla.org/mozilla-central/rev/ea1e180ed52c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.