Closed Bug 1469599 Opened 2 years ago Closed Last year

Debugger should show ES modules

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jlast, Assigned: jonco)

References

Details

Attachments

(1 file)

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
Blocks: 1472699
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.
Priority: -- → P3
Sounds good. I believe Ted had some ideas of where to get started here as well.
Flags: needinfo?(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|.
Flags: needinfo?(tcampbell)
This is almost certainly my fault.  I'll try and get to this tomorrow.
Assignee: nobody → jcoppeard
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.
Attachment #9003119 - Flags: review?(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"
Attachment #9003119 - Flags: review?(hsivonen) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1d6721cab0
Associate all module scripts in a module graph with DOM elements before execution r=hsivonen
It looks like the STR is still failing, but that likely means there are multiple underlying issues here.
https://hg.mozilla.org/mozilla-central/rev/3f1d6721cab0
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
re-opening as i can still reproduce the issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.