1765817 - Add flags to identify HTML (inline) sources

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

We need to remove breakpoint shifting first (bug 1755197).

This justify keeping the current implemention of syncBreakpoint method, which ought to be drastically simplified when switching to a breakpoint per URL world.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1765817 - fix original source breakpoint updates (to be meld into D138625)

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1765817 - [devtools] Simplify breakpoint list selectors.

The selector is already sorting the breakpoints,
so there is no need to re-sort them in the component.

Also Array.sort(stringA-stringB) was misbehaving.

Then I'm using getSourcesMap in order to avoid calling getBreakpointList many times.
Otherwise getSourcesForBreakpoints has to receive the state object
in order to retrieve the source objects.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1765817 - [devtools] Use getLocationSource selector to fetch the source object of any location.

This may help lookup by URL instead of source id in a subsequent patch.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1765817 - [devtools] Flag SOURCE resources for inline sources and flag debugger frontend sources for HTML sources. r=nchevobbe

In the frontend we end up having special sources, related to many source actors.
These are the sources for HTML pages, which have one unique source object.
But each inline <script> tag will have its own source actor.
All these sources actors will be bound to this unique source for the HTML page.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1765817 - [devtools] Migrate debugger selectors to breakpoint per source url (instead of only source id)

• 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.

Comment 7

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1765817 - [devtools] Simplify breakpoint list selectors r=nchevobbe

The selector is already sorting the breakpoints,
so there is no need to re-sort them in the component.

Also Array.sort(stringA-stringB) was misbehaving.

Then I'm using getSourcesMap in order to avoid calling getBreakpointList many times.
Otherwise getSourcesForBreakpoints has to receive the state object
in order to retrieve the source objects.

Comment 8

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1765817 - [devtools] Add getLocationSource selector to fetch source for a location r=nchevobbe

This cleanup helps support the subsequent work for breakpoints per url.

Depends on D146426

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9855f2e8144c
[devtools] Add getLocationSource selector to fetch source for a location r=nchevobbe

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9855f2e8144c

Comment 11

[Phabricator Automation]

Comment on attachment 9273410 [details]
Bug 1765817 - [devtools] Migrate debugger selectors to breakpoint per source url (instead of only source id)

Revision D144413 was moved to bug 1769787. Setting attachment 9273410 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9276679 [details]
Bug 1765817 - [devtools] Simplify breakpoint list selectors r=nchevobbe

Revision D146426 was moved to bug 1770204. Setting attachment 9276679 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9273409 [details]
Bug 1765817 - [devtools] Flag SOURCE resources for inline sources and flag debugger frontend sources for HTML sources. r=nchevobbe

Revision D144412 was moved to bug 1770205. Setting attachment 9273409 [details] to obsolete.