Closed Bug 1769787 Opened 3 years ago Closed 2 years ago

Implement breakpoints per URL

Categories

(DevTools :: Debugger, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
110 Branch

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.

  • 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't source-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.

Assignee: nobody → hmanilla
Status: NEW → ASSIGNED
Attachment #9276945 - Attachment description: Bug 1769787 - [devtools] Migrate debugger selectors to breakpoint per source url (instead of only source id) → Bug 1769787 - [devtools] Refactor to sources and breakpoints be url based r=
Attachment #9278091 - Attachment is obsolete: true
Attachment #9276945 - Attachment description: Bug 1769787 - [devtools] Refactor to sources and breakpoints be url based r= → Bug 1769787 - [devtools] Refactor to sources and breakpoints be url based
Blocks: 1734768
Attachment #9278091 - Attachment is obsolete: false
Blocks: 1780172
Severity: -- → S3
Priority: -- → P2

Depends on D150628

Depends on: 1785269
Depends on: 1785271
Depends on: 1785277

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.

Attachment #9290073 - Attachment is obsolete: true
Depends on: 1786269
No longer depends on: 1786269
Depends on: 1787198
Attachment #9278091 - Attachment description: Bug 1769787 - Make source id's no longer target specific → Bug 1769787 - [devtools] Make source id's no longer target specific r=ochameau
Attachment #9283472 - Attachment description: Bug 1769787 - [devtools] Make (target based) Source tree open tabs per url → Bug 1769787 - [devtools] Make (target based) Source tree open tabs per url r=ochameau
Depends on: 1798775
Depends on: 1800092
Depends on: 1800093
Blocks: 1802143
Blocks: 1805579
Pushed by hmanilla@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/96d47c799fbd [devtools] Make source id's no longer target specific r=ochameau https://hg.mozilla.org/integration/autoland/rev/e7b9d3d1ebd7 [devtools] Make (target based) Source tree open tabs per url r=ochameau

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

Regressions: 1812576
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

I believe this landed in Fx110.

Target Milestone: --- → 110 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: