1469599 - Debugger should show ES modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Laster [:jlast]]

It looks like we have a timing issue with how modules are loaded, which is affecting how the sources appear in the debugger.

STR:

1. go to https://cdn.rawgit.com/jakearchibald/a298d5af601982c338186cd355e624a8/raw/aaa2cbee9a5810d14b01ae965e52ecb9b2965a44/
2. add a breakpoint in (index) on line 6 addTextToBody
3. refresh
4. step into addTextToBody and pause in utils.js
5. add a breakpoint in utils.js#3
6. remove the breakpoint in (index)
7. refresh

Expected: pause in utils.js
Actual: don't pause, and  utils.js is not in the source tree

Comment 1

[Jason Orendorff [:jorendorff]]

Jason, if this is something you think we ought to just knock out, let me know in the weekly meeting. It may require some pair debugging to pin it down.

Comment 2

[Jason Laster [:jlast]]

Sounds good. I believe Ted had some ideas of where to get started here as well.

Comment 3

[Ted Campbell [:tcampbell]]

My suspicion is this may be related to Bug 1440269 where we added a mechanism to defer binding of the <script> element to the JSScript. This info is later used by the debugger's matching logic. See things hanging off |hideScriptFromDebugger|.

Comment 4

[Jon Coppeard (:jonco)]

This is almost certainly my fault.  I'll try and get to this tomorrow.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1469599-associate-module-graph

The problem was that the process of associating source elements with a script and exposing the script to the debugger only happened for the root module in a module graph.

The patch does this recursively for a module and all of its imports (module requests form a tree even when the graph has cycles).

I tested that this fixed the problem locally.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 9003119 [details] [diff] [review]
bug1469599-associate-module-graph

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

r+

::: dom/script/ScriptLoader.cpp
@@ +967,5 @@
>  nsresult
> +ScriptLoader::AssociateSourceElementsForModuleTree(JSContext* aCx,
> +                                                   ModuleLoadRequest* aRequest)
> +{
> +  // Preloaading can cause JS scripts to be compiled before DOM script element

Typo: "Preloaading"

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1d6721cab0
Associate all module scripts in a module graph with DOM elements before execution r=hsivonen

Comment 8

[Jason Laster [:jlast]]

It looks like the STR is still failing, but that likely means there are multiple underlying issues here.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3f1d6721cab0

Comment 10

[Jason Laster [:jlast]]

re-opening as i can still reproduce the issue.

Comment 11

[Jason Laster [:jlast]]

never mind, i was testing with artifact builds and forgot to wait for the patch to land on central.

Thanks for fix!

https://user-images.githubusercontent.com/254562/44485540-e8d06000-a61e-11e8-843b-ae35f269040d.png