Implement breakpoints per URL
Categories
(DevTools :: Debugger, enhancement, P2)
Tracking
(Not tracked)
People
(Reporter: bomsy, Assigned: bomsy)
References
(Depends on 3 open bugs, Blocks 5 open bugs)
Details
Attachments
(3 files, 1 obsolete file)
For now, the debugger frontend overall assume that breakpoint can be set per sourceId.
And because each target is having its own dedicated set of source IDs,
the same URL loaded in two distinc targets may have distinct set of breakpoints.
Unfortunately, on the backend, this doesn't work this way and instead breakpoint are global and per URL. You may set breakpoint per sourceId, but that's only for sources without URLs (like evals and console evaluations).
Because of this discrepencies between the two, we have various issues happening when setting breakpoints and interacting with more than one target.
For ex, bug 1734768 or bug 1755344 are symptoms of such issues.
We should revisit frontend assumptions to align on the backend's behavior.
This would actually align with the behavior of chrome. When you have the same URL loaded from a page and a worker, or same URL loaded twice in two workers, the breakpoints are synchronized. Setting a breakpoint in the first, will set a breakpoint in the second.
Assignee | ||
Comment 1•3 years ago
|
||
-
The one critical change made by this patch is the change in makeSourceId.
sourceId
is now always stable across reloads for sources with a URL.
And we still support sources without a URL, in which case we will use the Source Actor ID as sourceId.
So that sourceId are valid across reload and we no longer need to update the sourceId in breakpoint
persisted data (i.e. pending breakpoints).
And it can be used to designate both sources with URL as well as sources without URL. -
What I did in breakpoints.js selector is really gross.
We should probably replace breakpoints reducer and selector file with the content of pending breakpoints. -
Now that sourceId is stable across reload, we can actually use it in persisted storage.
The ID will actually make sense. This is why I modified makePendingLocationId to use it instead of sourceUrl.
But we should probably followup this change and ensure that we avoid storing in the persisted storage (asyncStorage)
all breakpoints against sources without a URL, i.e. whose location's sourceId isn'tsource-url-${url}
.
In this patch we will probably accumulate all these breakpoints which are meant to be transient
and not saved across reloads. -
This patch still breaks the SourceTree when the same URL is loaded in distinct targets/threads.
Because there is now one unique source per URL, only one is displayed in the SourceTree.
That's because the SourceTree is source object based, whereas it should probably
be source actor based.
See failure in browser_dbg-features-source-tree.js -
Also, pending breakpoints are now having id,text,originalText as that's useful.
May be text and originalTest shouldn't be stored in the persisted storage... -
In a few place I remove the usage of sourceUrl attribute on location.
We should really only be using sourceId everywhere!
So there is an overall removal of all usages of sourceUrl attribute. -
utils/breakpoint/index.js can probably be furher cleaned up
as we no longer have breakpoint versus pending breakpoint... -
syncBreakpoint is extensively simplified to cover only two very explicit cases:
- removing breakpoint when the line is no longer breakable
- update the generated location of breakpoints set on original sources
We may simplify this even more by having two distinct codepath for sourcemapped
and non-sourcemapped sources.
-
makeBreakpointLocation is now simplier, it no longer needs
state
as input,
it can now work solely based on sourceId. Also it makes sense to move it to commands module,
so that commands.setBreakpoint/removeBreakpoint/hasBreakpoint receive frontend locations
and internaly to this module we map them to server locations. -
Then, there is a few test changes as breakpoints are no longer cleared on navigation.
That's a significant change when we assert the reducers state.
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Updated•3 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
Depends on D144413
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
|
||
Depends on D150628
Comment 5•2 years ago
|
||
Comment on attachment 9290073 [details]
Bug 1769787 - Use source actor info for location selection (WIP)
Revision D154778 was moved to bug 1785277. Setting attachment 9290073 [details] to obsolete.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 7•2 years ago
|
||
bugherder |
Comment 8•2 years ago
|
||
Related prefherder alert, mostly improvements:
== Change summary for alert #36438 (as of Wed, 14 Dec 2022 12:49:38 GMT) ==
Regressions:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
14% | reload-debugger:parent-process objects-with-no-stacks | linux1804-64-qr | 3,317.62 -> 3,769.25 | |
5% | damp custom.jsdebugger.close.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 23.41 -> 24.61 |
5% | damp custom.jsdebugger.close.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 23.33 -> 24.44 |
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
10% | damp browser-toolbox.debugger-ready.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 1,020.73 -> 915.42 |
10% | damp browser-toolbox.debugger-ready.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 1,022.29 -> 919.49 |
8% | damp browser-toolbox.debugger-ready.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender | 1,072.92 -> 988.24 |
8% | damp browser-toolbox.debugger-ready.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 1,014.32 -> 935.54 |
8% | damp browser-toolbox.debugger-ready.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 1,059.86 -> 978.46 |
... | ... | ... | ... | ... |
3% | damp browser-toolbox.inspector-ready.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 414.55 -> 400.55 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=36438
Updated•2 years ago
|
Updated•2 years ago
|
Description
•