1713093 - Stop attaching actor from the client side

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Alexandre Poirot [:ochameau]]

When we start enabling server side targets, attaching from the client will no longer be necessary.
We should gradualy remove all attach sequences from the frontend as they become irrelevant.
This will ultimately allow removing the async behavior of TargetCommand, which doesn't notify about the target fronts immediately and instead wait for their attach requests to be processed.

This bug is about removing all the code called by this method:
https://searchfox.org/mozilla-central/rev/2b372b94ce057097a6ef8eb725f209faa9d1dc4d/devtools/client/fronts/targets/target-mixin.js#480-487

So that we can stop calling it from TargetCommand:
https://searchfox.org/mozilla-central/rev/2b372b94ce057097a6ef8eb725f209faa9d1dc4d/devtools/shared/commands/target/target-command.js#197-203

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Remove TargetMixin.attachThread.

This code was only used for workers targets, which weren't passing breakpoints
from the server side via "sessionData" concept.
Instead, we were still relying on the frontend to pass them and only after that
manually requesting the thread to resume its execution.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Fix intermittent with browser_inspector_command_findNodeFrontFromSelectors.

When loading the tab, an about:blank document is loaded first.
The top level target front may refer to that document instead of the test page.
Ensure waiting for that test page's target front before running the assertions.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Ensure processing all the target actors in xpcshell tests.

We were only processing the first one being registered.
Luckily we weren't testing session data enough in xpcshell to have trouble because of this.
(this changeset fixes test failure when applying the next one)

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Avoid exception while running browser_console.js

The new DevToolsProcess JS Actor will read the sharedData during its instantiation.
We should rather flush/persist sharedData before the JS Process Actor registration.
(this also fixes test failure when applying the next changeset)

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Allow using waitForSource to retrieve source object asynchronously.

waitForSource wasn't fully reliable to retrieve the source object.
Now that it is, we can remove a few useless usages of findSource following it.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Allow using waitForSource to retrieve source object asynchronously.

waitForSource wasn't fully reliable to retrieve the source object.
Now that it is, we can remove a few useless usages of findSource following it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0118d0bc64ba
[devtools] Fix intermittent with browser_inspector_command_findNodeFrontFromSelectors. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a63d603a1973
[devtools] Ensure processing all the target actors in xpcshell tests. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2670f26f6bff
[devtools] Avoid exception while running browser_console.js r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6532dbabf879
[devtools] Remove TargetMixin.attachThread. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b0285f8abf2e
[devtools] Allow using waitForSource to retrieve source object asynchronously. r=devtools-reviewers,nchevobbe

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0118d0bc64ba
https://hg.mozilla.org/mozilla-central/rev/a63d603a1973
https://hg.mozilla.org/mozilla-central/rev/2670f26f6bff
https://hg.mozilla.org/mozilla-central/rev/6532dbabf879
https://hg.mozilla.org/mozilla-central/rev/b0285f8abf2e

Comment 9

[pstanciu]

Backed out for causing Bug 1767717 into permafail.

• Backout link
• Push with failures
• Failure Log

Could you please take a look?
Thank you!

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Ensure applying Session Data updates received while Service Worker targets are being instantiated.

The onConnectToWorker is async and we may receive new Session Data (typically breakpoints)
while we are instantiating the worker targets.

Add some dedicated Set to collect these concurrent Session Data updates happening while
some targets are being instantiated.

In this patch, I'm only applying this logic to Service Workers, but I'll try to apply this logic to all targets types in bug 1866814.
Also, ideally, Session Data should be applied before watching the targets.
Breakpoints would ideally be passed on toolbox opening, before watching for targets.
But for now, they are being passed by the Debugger Frontend, whereas Target watching is done earlier by the Toolbox.

Comment 11

[Julian Descottes [:jdescottes]]

For the record, I think that the spike on Bug 1767739 - Intermittent devtools/client/storage/test/browser_storage_cookies_navigation.js | single tracking bug could be related to the patches that temporarily landed here. To monitor when re-landing.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1713093 - [devtools] Wait for indexedDB iniatialization before navigating in browser_storage_cookies_navigation.js.

Unfortunately navigateTo isn't waiting for full initialization of the storage
panel when navigating to another page.
It appears that indexedDB is slightly more asynchronous as should be manually waited for.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

FYI when the patch first landed, this caused a pretty big regression on styleeditor reload: https://treeherder.mozilla.org/perfherder/alerts?id=41694&hideDwnToInv=0

It would be interesting to check DAMP before landing the patches

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #11)

For the record, I think that the spike on Bug 1767739 - Intermittent devtools/client/storage/test/browser_storage_cookies_navigation.js | single tracking bug could be related to the patches that temporarily landed here. To monitor when re-landing.

Fixed thanks to an additional changeset:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=dek1KwdTQbartkoqVljZCw.0&revision=b9a5206f1cd86aeaceac67d36ed292ae9f118d91

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)

FYI when the patch first landed, this caused a pretty big regression on styleeditor reload: https://treeherder.mozilla.org/perfherder/alerts?id=41694&hideDwnToInv=0

It would be interesting to check DAMP before landing the patches

Latest revision doesn't seem to regress the performance?

https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=c6a4531677ff6613d22bcc69d8abc681d6fd0092&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=f40dcaac36382ecc75fac909e1919460ed09303a&page=1&showOnlyConfident=1

I'll probably wait for the merge before landing this patch.

Comment 15

[Alexandre Poirot [:ochameau]]

OH wait, I only ran debugger damp tests. With the styleditor I see a 3%regression on styleditor complicated reload:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=c6a4531677ff6613d22bcc69d8abc681d6fd0092&originalSignature=4943452&newSignature=4943452&framework=12&application=firefox&originalRevision=f40dcaac36382ecc75fac909e1919460ed09303a&page=1&showOnlyConfident=1

Comment 16

[Alexandre Poirot [:ochameau]]

Thanks to bug 1885437, there is no more impact on style editor DAMP tests.
Various small improvements and now only a 17% regression on custom.netmonitor.reload:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=53262ea1108a637b412534156e588acf09ef53f1&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=94e2cc1e39883c971998b0bfc9d0f4cff34450b5&page=1&showOnlyConfident=1

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/935858ea4146
[devtools] Fix intermittent with browser_inspector_command_findNodeFrontFromSelectors. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/75eb6b1a4c6c
[devtools] Ensure processing all the target actors in xpcshell tests. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/11e17b5c1692
[devtools] Avoid exception while running browser_console.js r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b1ec8afe5e13
[devtools] Remove TargetMixin.attachThread. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6d28e4b76c0c
[devtools] Allow using waitForSource to retrieve source object asynchronously. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/5c9567b96aaf
[devtools] Ensure applying Session Data updates received while Service Worker targets are being instantiated. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/8d016596b0cd
[devtools] Wait for indexedDB iniatialization before navigating in browser_storage_cookies_navigation.js. r=devtools-reviewers,nchevobbe

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/935858ea4146
https://hg.mozilla.org/mozilla-central/rev/75eb6b1a4c6c
https://hg.mozilla.org/mozilla-central/rev/11e17b5c1692
https://hg.mozilla.org/mozilla-central/rev/b1ec8afe5e13
https://hg.mozilla.org/mozilla-central/rev/6d28e4b76c0c
https://hg.mozilla.org/mozilla-central/rev/5c9567b96aaf
https://hg.mozilla.org/mozilla-central/rev/8d016596b0cd

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

== Change summary for alert #41968 (as of Fri, 22 Mar 2024 05:06:27 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	damp console.log-in-loop-content-process-string	linux1804-64-shippable-qr	e10s fission stylo webrender	65.63 -> 60.16
7%	damp console.log-in-loop-content-process-number	linux1804-64-shippable-qr	e10s fission stylo webrender	54.00 -> 49.99
5%	damp console.log-in-loop-content-process-null	windows10-64-shippable-qr	e10s fission stylo webrender	38.69 -> 36.60
5%	damp console.log-in-loop-content-process-nan	linux1804-64-shippable-qr	e10s fission stylo webrender	41.12 -> 38.99
5%	damp console.log-in-loop-content-process-longstring	windows10-64-shippable-qr	e10s fission stylo webrender	46.36 -> 44.13
...	...	...	...	...
2%	damp console.log-in-loop-content-process-object	windows10-64-shippable-qr	e10s fission stylo webrender	210.01 -> 205.29

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41968