1605686 - cannot put breakpoint on top level ES module files

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Pierre Ossman]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. Open debugger
2. Right click on a top level line (i.e. not in a function)
3. Select "Add Breakpoint"

Actual results:

Nothing at all

Expected results:

A breakpoint should have been set and triggered on the next reload.

Comment 1

[Pierre Ossman]

This is a massive issue as it makes debugging in Firefox impossible for lots of cases. Not sure how this snuck under the radar.

Oddly enough it doesn't happen on every file, but many. Example here:

https://novnc.com/noVNC/vnc_lite.html

The script tag on the page itself is broken, as are numerous files it loads.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jlast, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Jason Laster [:jlast]]

Hi Pierre, thank you for filing. We take registering breakpoints at the top-level seriously and have many tests for that functionality.

Can you share a couple locations in https://novnc.com/noVNC/vnc_lite.html where it is not working for you?

Comment 5

[Pierre Ossman]

Attachment: Screenshot from 2020-01-08 12-26-12.png

See attached image. In the <script> tag in vnc_lite.html there is not a single top level line where I can add a breakpoint. The only possible lines are those in functions. E.g. line 177 would be very interesting to debug, but isn't available:

    rfb = new RFB(document.getElementById('screen'), url,
                  { credentials: { password: password } });

Comment 6

[Pierre Ossman]

Ehm.... Just to make things extra confusing, I just closed that tab and opened it again. And now suddenly I can set breakpoints on those lines. So the issue is intermittent I guess.

Some race? Some cache when the scripts are modules?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jlast, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Logan Smyth [:loganfsmyth]]

I'm guessing this is an bug with the logic we added in https://bugzilla.mozilla.org/show_bug.cgi?id=1572280. From what I can see, we're failing the condition in https://searchfox.org/mozilla-central/rev/be7d1f2d52dd9474ca2df145190a817614c924e4/devtools/server/actors/source.js#449 so we never bother reparsing.

@brian
I'm guessing that since these are modules, we get something something that is actually a function top-level? If you don't know, I can look into this, just curious for your thoughts.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth][PTO Dec 26-Jan 3] from comment #8)

I'm guessing this is an bug with the logic we added in https://bugzilla.mozilla.org/show_bug.cgi?id=1572280. From what I can see, we're failing the condition in https://searchfox.org/mozilla-central/rev/be7d1f2d52dd9474ca2df145190a817614c924e4/devtools/server/actors/source.js#449 so we never bother reparsing.

@brian
I'm guessing that since these are modules, we get something something that is actually a function top-level? If you don't know, I can look into this, just curious for your thoughts.

Yeah, it does seem like the GC has collected the top level script and that we aren't reparsing it right. I suspect the problem is that the reparse() call is throwing because the script is being reparsed like a normal script instead of a module (it has module specific syntax import). The thrown exception causes the old set of scripts to be used, so we will only be able to set breakpoints in scripts that haven't been GC'ed. See also bug 1579654 which might be the same issue.

Comment 10

[Logan Smyth [:loganfsmyth]]

Yep, Brian totally called it, I guess I misidentified the specifics when I was exploring yesterday. We see that there is no top-level Script (since it was GCed), but when we try to reparse, it throws an exception since it is reparsing as a script instead of a module.

Glancing over things, this could be a little complex to fix because it doesn't look like Debugger.Source actually has a way to know what type the original parser pass used? That data is attached to Scripts, so only the root script would have known the type, and in this context it has been GCed. We'd need to store that data and then use it in reparse.

Comment 11

[Logan Smyth [:loganfsmyth]]

@Brian Not sure how your queue is looking, but is this something you'd want to look into? I'd really like to not context-switch off of async stacks too much right now and this is much more work that I was expecting. Otherwise perhaps this can wait a bit?

@Pierre

Ehm.... Just to make things extra confusing, I just closed that tab and opened it again. And now suddenly I can set breakpoints on those lines. So the issue is intermittent I guess.
Some race? Some cache when the scripts are modules?

What is happening is that, because the devtools were not open when the page loaded, the information about where breakpoints are allowed (for some partial piece of your code anyway), has been deleted by the garbage collector. We have logic to detect this case and rebuild that information when the debugger is opened, but that logic is failing because it cannot handle type="module" script tags. When you refresh the page, the debugger is already loaded and active, so it prevents the information from being GCed, which is why you are seeing it work after a refresh.

Comment 12

[Brian Hackett [Laid off!]]

Sure, I can take a look this week.

Comment 13

[Brian Hackett [Laid off!]]

I had to iterate on this a bit but there is what seems a simple solution: just try to reparse all scripts as modules, because AFAIK any valid top level script is also a valid module. If that isn't the case we can add a flag argument to reparse() for how to reparse the script, and just try it both ways in the server.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: Bug 1605686 - Reparse scripts as modules, r=loganfsmyth.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jlast, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Attachment: WIP: Bug 1605686 - [devtools] Add tests for adding breakpoint on GCd modules.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Bryan, is this something you could help us with ?

Basically in https://searchfox.org/mozilla-central/rev/058ab60e5020d7c5c98cf82d298aa84626e0cd79/devtools/server/actors/source.js#374-380

try {
  newScript = this._source.reparse();
} catch (e) {
  // reparse() will throw if the source is not valid JS. This can happen
  // if this source is the resurrection of a GC'ed source and there are
  // parse errors in the refetched contents.
}

reparse throws with SyntaxError: import declarations may only appear at top level of a module when the script is a module, as reparse doesn't work on module (as far as I understand).
There was an attempt to fix this adding the ability to reparse modules (https://phabricator.services.mozilla.com/D59955), but Brian was fired before it could address the comments there.

I pushed a patch with a test that demonstrates the issue (https://phabricator.services.mozilla.com/D207243) as Brian's patch doesn't apply cleanly, and was missing forcing the gc from within the module script itself.

Comment 19

[Bryan Thrall [:bthrall]]

I'm happy to take a look!

Comment 20

[Bryan Thrall [:bthrall]]

Attachment: Bug 1605686 - Reparse scripts as modules r=nchevobbe

I merged Brian Hackett's original patch (D59955) with Alex's test improvements
(D207243) and rebased ontocentral.

Comment 21

[Bryan Thrall [:bthrall]]

I plan to add tests for non-strict JS, eval, and new Function() before asking for a code review.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Bryan, can you tell us why you did change the priority on this? I feel like this is still an important bug that we should try to fix soon-ish, but I guess there's a good reason you updated the priority

Comment 23

[Bryan Thrall [:bthrall]]

I'm sorry, I made this change without thinking it through. I changed it only based on my own priorities and should have considered how your team prioritizes things.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Bryan Thrall [:bthrall] from comment #23)

I'm sorry, I made this change without thinking it through. I changed it only based on my own priorities and should have considered how your team prioritizes things.

no worries :)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e54ad129dfd2
https://hg.mozilla.org/integration/autoland/rev/b1b8f3ff7c8b
Reparse scripts as modules r=nchevobbe,devtools-reviewers,arai

Comment 26

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/b1b8f3ff7c8b