1627712 - Browser Toolbox Debugger not stopping for breakpoints in browser.js ' UpdateUrlbarSearchSplitter

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[:Gijs (he/him)]

What were you doing?

I'm trying to fix bug 1627685. Step 0 is figuring out what's happening now, so:

1. ./mach run --temp-profile (or start nightly with a clean profile and enable browser debugger, same thing)
2. open the prefs (cmd-comma on mac)
3. switch to 'search' section
4. enable search bar
5. open browser toolbox
6. switch to debugger
7. accel-p to bring up the filter box, type in 'browser.js', pick the first hit (chrome://browser/content/browser.js )
8. accel-f to bring up the find bar, search for "urlbarsearch"
9. go to the second hit, which is on function UpdateUrlbarSearchSplitterState() {
10. set breakpoints on any of the lines in the function
11. switch back to the browser window
12. drag the right window edge all the way to the left, to make the window much narrower

What happened?

The debugger did not stop for the breakpoint.

What should have happened?

The debugger should have hit the breakpoint

Anything else we should know?

If I use a debugger; statement, the debugger stops, so the code is clearly getting hit.

Between this and bug 1613376, it is really hard to do any debugging of browser code. These are day-to-day tools, they need to be more reliable.

Comment 1

[Logan Smyth [:loganfsmyth]]

This is caused by an optimization that isn't correct: https://searchfox.org/mozilla-central/rev/9120151ddb35f2d4c37bfe613a54a4f10a9a3dc5/devtools/server/actors/source.js#399-401

If a file is executed multiple times, it may have the same Debugger.Source, so you can have one evaulation of the source where there is a "toplevel" script, and one where there isn't, and there isn't any way to tell those apart. The optimization performed on that line assumes that if there is a top-level script, it can be used, but that is at risk of discarding whole subsets of scripts where they have no top-level script because there were 2 copies, one where the top-level was GCed and one where it wasn't. e.g.

1. Load "browser.js"
2. Clone "browser.js"
3. The top-level JSScript of the cloned "browser.js" is GCed
4. Something in the debugger calls "_getTopLevelDebuggeeScripts"

Since there is a top-level script still for the first copy of "browser.js" that was evaluated, we skip the potentially-slow logic of

for (const script of allScripts) {
  for (const child of script.getChildScripts()) {
    allScripts.delete(child);
  }
}

but that means that the returned scripts array entirely excludes JSScripts created on step 2, so a whole file's worth of JSScripts are ignored during breakpoint processing if there are multiple copies of a file evaluated using the same ScriptSourceObject and and on some have had their root JSScript GCed and some have not, we'll hit this but. And usually in this scenario that is likely because of the way Gecko does script caching, the original always has its root held strongly, so if nothing in the script explicitly causes the root to stay alive, it will get cloned and run, then the GC will run and delete the root of the duplicate, so this bug is very likely for any script that is may load multiple times (potentially this would only happen if the script is loaded twice in the same compartment though).

Comment 3

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1627712 - Consistently allow breakpoints in sources evaluated multiple times. r=jlast!

Comment 4

[:Gijs (he/him)]

Thanks for investigating and fixing this! Really looking forward to it.

I have two questions:

1. would it be possible to write automated tests for some of this to ensure we don't regress?
2. comment #1 mentions we run browser.js multiple times and one of them gets GC'd. With my performance hat on, I'm interested: where do we run it that gets GC'd? IIRC it runs in both the browser window and the hidden window, but I'd expect both of those to stay alive... is it just the JSScript instance that goes away, while the resulting functions etc. stay available?

Comment 5

[Logan Smyth [:loganfsmyth]]

would it be possible to write automated tests for some of this to ensure we don't regress?

I can look into it, I'm not familiar with writing tests for the toolbox so I don't know if that's easy or what. If it is an option, I'd be happy to though. I threw the patch up as-is since it's the end of the week for now.

IIRC it runs in both the browser window and the hidden window, but I'd expect both of those to stay alive... is it just the JSScript instance that goes away, while the resulting functions etc. stay available?

It's not that the whole file is GCed, but the outermost JSScript of the file. Each file has a tree of JSScripts that is basically the tree of function declarations, and they store the bytecode that will run when a particular piece of code is executed. Since the top-most script only ever runs the first time a file is evaluated, often it ends up being GCed because nothing references it since it can never run a second time. There are still JSScript objects for any function in the file that is still reachable though.

So we have:

1. browser.js loads and is cached in Firefox's in-memory cache of JSScripts for resource:// and chrome:// file, so it doesn't have to re-parse the JS files if they load multiple times
2. browser.js loads a second time, and the JSScript and all it's children are cloned and then it is evaluated.
3. GC runs and the top-level script returned by the clone step is GCed since it can never run again

If you analyze the 2 sets of JSScripts associated with browser.js now, the first one has a top-level script because it is held alive by Firefox's cache, and the second one does not. The optimization I removed will currently assume that the top-level script for the JSScripts from step 1 can be traversed to reach all JSScripts for browser.js, but that is not the case because traversing that script will not reach any of the scripts created in step 2.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/974cf9260bad
Consistently allow breakpoints in sources evaluated multiple times. r=jlast

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/974cf9260bad