912719 - Off-thread compilation breaks Debugger onNewScript notifications

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The off-thread compilation changes (JS::CompileOffThread and friends) don't ensure that Debugger's onNewScript notifications get sent. Those notifications are the only change debuggers have to insert breakpoints in code before it runs.

Here's a test, based on the shell enhancements in bug 912321, that fails in the current sources.


// We still get onNewScript notifications for code compiled off the main thread.

var g = newGlobal();
var dbg = new Debugger(g);

var log;
dbg.onNewScript = function (s) {
  log += 's';
  assertEq(s.source.text, '"t" + "wine"');
}

log = '';
g.offThreadCompileScript('"t" + "wine"');
assertEq(runOffThreadScript(), 'twine');
assertEq(log, 's');

Comment 1

[Luke Wagner [:luke]]

Brian: is this being fixed along with the other current issues with off-main-thread parsing like error reporting for bug 906371?

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

This fixes the onNewScript notification issue, as well as bug 913519 --- the two are somewhat entangled due to bug 910744.  I meant to fix this as part of bug 897655 but forgot to.

Off thread error reporting is already in place (bug 908699) and right now is only turned off for XUL documents because using AutoSafeJSContext breaks some tests (bug 910744).  This patch includes a workaround for that issue, by only instantiating AutoSafeJSContext in cases where a JSContext is actually needed.  Tests pass on try with this patch.

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Do you mean to use MOZ_CRASH instead of MOZ_ASSUME_UNREACHABLE?  IIUC, the behavior of actually running MOZ_ASSUME_UNREACHABLE in an opt build is undefined.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Do you mean to use MOZ_CRASH instead of MOZ_ASSUME_UNREACHABLE?  IIUC, the
> behavior of actually running MOZ_ASSUME_UNREACHABLE in an opt build is
> undefined.

MOZ_CRASH would be fine to use, though these MOZ_ASSUME_UNREACHABLE are there due to a local invariant --- no one should be doing off thread compilation stuff unless CanCompileOffThread returned true, which it won't do in non-threadsafe builds.  The JS engine is peppered with several hundred MOZ_ASSUME_UNREACHABLE.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bobby suggested a possible fix for bug 910744. He thinks that the AutoSafeJSContext should be scoped so that it wraps only FinishOffThreadScript and not OnScriptCompileComplete. Apparently it causes changes in behavior that could be causing stuff to not run.

Brian, could you see if this works on try (or locally)?

Comment 6

[Brian Hackett [Laid off!]]

Attachment: updated

Cool, bholley's fix works both locally and on try.  Here is an updated patch that still fixes both this and bug 913519 (though the changes are now independent).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 802305 [details] [diff] [review]
updated

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

::: js/src/jsworkers.cpp
@@ +502,5 @@
>  
> +static void
> +CallNewScriptHookForAllScripts(JSContext *cx, HandleScript script)
> +{
> +    JS_ASSERT(cx->compartment()->debugMode());

I think this should be changed. See below.

@@ +510,5 @@
> +    JS_CHECK_RECURSION(cx, return);
> +
> +    // The global new script hook is called on every script that was compiled.
> +    RootedFunction function(cx, script->function());
> +    CallNewScriptHook(cx, script, function);

Based on a quick reading of the existing code, it looks like we call the hook for the outer script *after* the hook for the inner scripts. Can you preserve that behavior by moving this call below the loop? It may not matter, but it can't hurt.

@@ +586,1 @@
>      if (maybecx) {

Are there any cases where we don't have a cx anymore? If not, can you just require it to be non-null?

@@ +587,5 @@
>          AutoCompartment ac(maybecx, parseTask->scopeChain);
>          for (size_t i = 0; i < parseTask->errors.length(); i++)
>              parseTask->errors[i]->throwError(maybecx);
> +
> +        if (script && maybecx->compartment()->debugMode()) {

Are you sure that the newScriptHook will only be set when debugMode is set? I don't see anything that suggests that. It seems like we could instead check if cx->runtime()->debugHooks.newScriptHook is non-null when calling CallNewScriptHookForAllScripts.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, and can you check in Jim's test as well?

Comment 9

[Brian Hackett [Laid off!]]

Jim's test depends on bug 912321, which hasn't landed yet.

https://hg.mozilla.org/integration/mozilla-inbound/rev/55b09bed4122

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55b09bed4122