1503422 - Debugger doesn't see sources that are cleaned up by GC

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[David Walsh :davidwalsh]

There are cases where files with the same name but different query string values don't display in the debugger.  Example:

https://davidwalsh.name/demo/devtools-qstring.php

The above page receives the following scripts:

EmptyLineTest.js
EmptyLineTest.js?x=1
EmptyLineTest.js?x=2
EmptyLineTest.js?y=1

Sometimes the debugger only receives one of the those scripts.  This leads to the following intermittent failure:

https://github.com/devtools-html/debugger.html/issues/7185

And we should always get every script, as each loads and executes an onload, as my example shows.

Comment 1

[David Walsh :davidwalsh]

I made the following change to the debugger.html to let me know what was coming down:

```js
function newSource(_: "newSource", { source }: SourcePacket) {
+  console.log("Received new source!", source);
```

After a bunch of refreshes on https://davidwalsh.name/demo/devtools-qstring.php, I got a case where only one of the scripts was displaying in the tree.  I analyzed the console logs and only one was received by the client ("y=1")

I did the same for https://davidwalsh.name/demo/devtools-qstring-contribute.php, and couldn't reproduce the issue -- all querystring'd sources displayed in the tree each time.

Comment 2

[David Walsh :davidwalsh]

Pinging Jim on this so we don't lose track of it.

Comment 3

[Jim Blandy :jimb]

My guess is that this and bug 1486483 are caused by the GC cleaning things up that didn't leave a trace, before the devtools ever see it. Devtools may still want to set a breakpoint in a script, even if it's short-lived, so the GC cleaning it up is a problem. When I can get to this, I'll verify that this is really what's going on. If so, we need to ask the SpiderMonkey team if they're comfortable spending the memory to keep around the URLs. We can put a cap on it --- nobody needs to see five hundred URLs, and nobody needs to see kilobyte-long URLs, so we can bound the memory it would use.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb (Away from bugmail Dec 24-Jan 1) from comment #3)
> My guess is that this and bug 1486483 are caused by the GC cleaning things
> up that didn't leave a trace, before the devtools ever see it. Devtools may
> still want to set a breakpoint in a script, even if it's short-lived, so the
> GC cleaning it up is a problem. When I can get to this, I'll verify that
> this is really what's going on. If so, we need to ask the SpiderMonkey team
> if they're comfortable spending the memory to keep around the URLs. We can
> put a cap on it --- nobody needs to see five hundred URLs, and nobody needs
> to see kilobyte-long URLs, so we can bound the memory it would use.

I think this diagnosis is correct.  I can reproduce this by opening the page, visiting about:memory in another tab, clicking 'Minimize memory usage' to trigger several GCs/CCs, and then opening devtools in the original tab.  Only EmptyLineTest.js (i.e. not with any query string values) is displayed.

There should be other ways to deal with this than keeping old URLs around in case the devtools are opened later.  A simple workaround is to just have the devtools open when loading the page --- the sources seem to be kept alive and can still be accessed from the client even after minimizing memory usage.  A next, smaller step could be to keep track of the number of sources that have been generated for a page, and if we detect that some have been GC'ed (the number of found sources is less than the number of created ones), show that to the user somehow ('Reload to see missing sources' at the bottom of the sources pane, maybe?).

Comment 5

[Jason Laster [:jlast]]

> Reload to see missing sources' at the bottom of the sources pane

this seems like a good solution.

Comment 6

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #4)
> There should be other ways to deal with this than keeping old URLs around in
> case the devtools are opened later.  A simple workaround is to just have the
> devtools open when loading the page --- the sources seem to be kept alive
> and can still be accessed from the client even after minimizing memory
> usage.

Yes, I think the server's onNewScript hook takes explicit steps to retain them (simply by holding pointers to them in a table). I don't remember where the code is, though.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: WIP

This patch implements the idea in comment 4. If the debugger attaches and we detect that sources have been GC'ed, a message is shown at the bottom of the sources pane. Reloading or navigating with the debugger open will clear the message.

This works but I think we can do better in terms of detecting whether meaningful script sources have been collected. This will fire if any sources have been collected by the process, including those from pages prior to the user navigating (including the initial contents of a new tab). Showing the message at inappropriate times will be unnecessarily confusing for users.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: screenshot

Comment 11

[Brian Hackett [Laid off!]]

Attachment: patch

The strategy in this patch works pretty well from the testing I've done, i.e. reporting that there is a missing source at the expected times. This shows the 'Reload to see missing sources' message in the sources pane if at the time the debugger asked for the sources, there is a source meeting the following criteria:

• The source was created for the main thread's runtime since the last page navigation (including going back/forward, reloading, and visiting a new URI).
• The source matches a filter looking for content scripts.
• The source is no longer reachable from any scripts in the main thread's runtime.

Doing things this way avoids needing to keep track of URLs or other information associated with unreachable sources that would need careful management.

This is trying to keep things as simple as possible until there is some feedback about how it could be improved. Note that this can incorrectly report both the presence and absence of missing sources:

• If a source has been collected that the debugger doesn't care about, like an eval'ed source with no source map info, we will incorrectly report the presence of missing sources.

• If a page uses a content source from before navigating, if that source is collected then we can incorrectly report the absence of missing sources. I think this can happen when going back/forward (not sure about otherwise), but it seems difficult to trigger in any case.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add Debugger.hasCollectedSources().

Add a Debugger method to determine whether sources that might be relevant to the debugger are unreachable, according to the description in comment 11.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Notify JS when navigating.

This patch uses the API added in part 1 to notify the JS engine's debugger when a page navigates (goes back/forward, reloads, or visits a new URI).

Comment 14

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Show message when there might be missing sources.

Devtools server/client changes to determine whether there might be missing sources in the main thread's runtime and show a message in the sources pane.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9048707 [details] [diff] [review]
Part 2 - Notify JS when navigating.

Do we need to care about bfcached pages? 
Somewhere around https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/docshell/base/nsDocShell.cpp#7238

Do we need to deal with navigations in iframes somehow differently?
And...OnNavigate is static. So if a background tab navigates, we still notify. Is that all expected?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(I couldn't set superreview flag to anything, so just cleared the name)

Comment 17

[Brian Hackett [Laid off!]]

(In reply to Olli Pettay [:smaug] from comment #15)

Comment on attachment 9048707 [details] [diff] [review]
Part 2 - Notify JS when navigating.

Do we need to care about bfcached pages?
Somewhere around
https://searchfox.org/mozilla-central/rev/
3e0f1d95fcf8832413457e3bec802113bdd1f8e8/docshell/base/nsDocShell.cpp#7238

Do we need to deal with navigations in iframes somehow differently?
And...OnNavigate is static. So if a background tab navigates, we still
notify. Is that all expected?

This is expected, but you're right that the approach has a lot of holes and will only work well in certain circumstances, e.g. visiting a page in a new/existing tab and then opening the debugger. I think we can do better by keeping track of this information at the realm level, which both syncs up better with the debugger's state (debuggees are per-realm) and should avoid needing to know about navigations (since the realms which the debugger will find change when we navigate).

Comment 18

[Brian Hackett [Laid off!]]

Comment on attachment 9048706 [details] [diff] [review]
Part 1 - Add Debugger.hasCollectedSources().

I'll work on an updated patch that tracks information at the realm level.  (Part 3 shouldn't need any changes though.)

Comment 19

[Brian Hackett [Laid off!]]

Comment on attachment 9048709 [details] [diff] [review]
Part 3 - Show message when there might be missing sources.

Actually, per recent discussion, showing a message instead of the sources seems more confusing than anything else for users.  It seems best to table this for now until we can decide on a better solution.  The patches here should still be useful for developing a realm-based approach for detecting missing sources.

Comment 20

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add Debugger.hasCollectedSources().

Updated realm-based patch, for posterity.

Comment 21

[:Harald Kirschner :digitarald]

:bhacket, looks great. As a comparison, Chrome has something similar in their Security panel.

A follow up I need to explore is opening GCd sources (from the Console for example) in Debugger, that then are non-debuggable as the server isn't aware of them. Is there a bug about those, or am I misunderstanding the case?

Comment 22

[Brian Hackett [Laid off!]]

(In reply to :Harald Kirschner :digitarald from comment #21)

:bhacket, looks great. As a comparison, Chrome has something similar in their Security panel.

A follow up I need to explore is opening GCd sources (from the Console for example) in Debugger, that then are non-debuggable as the server isn't aware of them. Is there a bug about those, or am I misunderstanding the case?

Bug 1347018 has STR for this case, where a console link may go to either the debugger or view-source, depending on whether the underlying source has been GC'ed.

Comment 23

[Logan Smyth [:loganfsmyth]]

This should be addressed with https://bugzilla.mozilla.org/show_bug.cgi?id=1572596