1132224 - [jsdbg2] all child scripts don't exist when `onNewScript` is called

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

The bug on the devtools side for this is bug 1131174, but I'll explain it again here.

The test case (on nightly):

1. Go to http://webaudiotool.com/app/?patch=1C170F35-3D45-0777-0EFF-8F3DFE42A3F1 (probably want to mute)
2. Open debugger, select AudioContextManager.js
3. Set a breakpoint on line 54, reload, and you'll see it breaks completely on the wrong place

This is because when a page is reloaded, we get `onNewScript` notifications for top-level scripts, and in there we re-set all the existing breakpoints on all the necessary scripts.

The problem is that for some reason, in this case, we aren't getting all the nested child script when we are asking for it. Beta has different behavior (jsantell said it works there, but for me it just doesn't hit the breakpoint at all, so I don't know).

The difference between beta and nightly/aurora is that we changed how we load the child scripts. We used to use `findScripts` in the breakpoint setting logic. But now we eagerly do `findScripts` early and keep hard references to all the scripts to avoid GC timing issues. This technique is a *huge* boon for us.

So basically now the logic is:

onNewScript: function(script, global) {
  let scripts = this.dbg.findScripts({ source: script.source });
  this.scripts.addScripts(scripts);

  this._addScript(script);
}

Don't worry about `_addScript`, it basically just fires off the breakpoint setting logic, which calls into our ScriptStore `this.scripts` to get the scripts.

Unfortunately it seems like using `findScripts` here doesn't necessarily return all the scripts, when I thought that we definitely guaranteed that. If you log the URL and number of scripts returned from the above `findScripts` call, AudioContextManager.js only gets 1 script, when it should get 5.

In fact, it's especially enlightening to change the above code to:

onNewScript: function(script, global) {
  let scripts = this.dbg.findScripts();
  let found = s.filter(script => script.source.url &&
                       script.source.url.indexOf('AudioContextManager.js') !== -1);
  if(found.length > 0) {
    console.log('onNewScript found', found.length, aScript.source.url);
  }

  this.scripts.addScripts(scripts);
  this._addScript(script);
}

For every `onNewScript` call, we log the number of scripts that a global `findScripts` returns that are attached to AudioContextManager.js (we also log the script URL that triggered this call). This should definitely show 5 scripts for all of the logs, but weirdly this is what it shows:

console.log: onNewScript found 1 http://webaudiotool.com/app/js/AudioContextManager.js?v=11
console.log: onNewScript found 2 http://webaudiotool.com/app/js/ModuleEvent.js?v=11
console.log: onNewScript found 2 http://webaudiotool.com/app/js/ConnectionSelectionGrid.js?v=11
console.log: onNewScript found 2 http://webaudiotool.com/app/js/MenuConfig.js?v=11
console.log: onNewScript found 5 http://webaudiotool.com/app/js/AbstractOverlayElement.js?v=11
console.log: onNewScript found 5 http://webaudiotool.com/app/js/AudioContextManagerEvent.js?v=11
console.log: onNewScript found 5 http://webaudiotool.com/app/js/Utils.js?v=11
console.log: onNewScript found 5 http://webaudiotool.com/app/js/CodeGenerator.js?v=11

For some weird reason, the child scripts don't seem to come into existence until a later time.

Comment 1

[James Long (:jlongster)]

Also, I tried to simplify this into a smaller test case, but of course, when I do that the problem goes away and it shows 5 child scripts immediately.

Comment 2

[James Long (:jlongster)]

Another data point: calling `getChildScripts` does force the scripts into existence. A while back I concluded that `getChildScripts` does a bit more work because it seemed a good bit slower than doing the `findScripts` query. But trying it now, it's not that bad, and my perf hit may have been from other stuff.

Still, I'm super curious what the different is of recursively calling `getChildScripts` and `findScripts({ source: mySource })`

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Reword delazification method names and remove stale reference to "debug mode".

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Propagate the delazification for Debugger flag when merging compartments.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Test.

Comment 6

[Shu-yu Guo [:shu]]

This platform bug has been there since 2013, but from comment 1 it seems like we only need to backport to Aurora.

Comment 7

[James Long (:jlongster)]

Awesome! Yeah, just uplifting to Aurora should be fine.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8563143 [details] [diff] [review]
Reword delazification method names and remove stale reference to "debug mode".

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

I'll bet this one has bit-rotted with the unobserved asm.js flag patch, but the name changes look good to me.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8563144 [details] [diff] [review]
Propagate the delazification for Debugger flag when merging compartments.

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

::: js/src/jsgc.cpp
@@ +6505,5 @@
> +    // The delazification flag indicates the presence of LazyScripts in a
> +    // compartment for the Debugger API, so if the source compartment created
> +    // LazyScripts, the flag must be propagated to the target compartment.
> +    if (source->needsDelazificationForDebugger())
> +        target->scheduleDelazificationForDebugger();

Painful in its simplicity.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b376024e3e0
https://hg.mozilla.org/mozilla-central/rev/c9fd3ca16a29
https://hg.mozilla.org/mozilla-central/rev/640793eb1169