1651518 - Enable all server side worker targets for the browser toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

Bug 1633712 will introduce the first framework code in order to create WorkerTargetActor's via the Watcher Actor.
But it will most likely do that only for "regular workers", i.e. workers from a web page, running in the process of the page. This will most likely not cover workers running from the parent process, nor service workers, nor shared workers.

So this one bug will be about following up in order to support parent process workers and shared workers (which, I think all runs in the parent process?).
It might end up being trivial and be only about tweaking a few filtering methods from the code landed in bug 1633712. But may be not?

And we should ensure having a proper coverage for these workers in:
https://searchfox.org/mozilla-central/source/devtools/shared/resources/tests/browser_target_list_workers.js

Comment 1

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c).

Comment 2

[Neha Kochar [:neha]]

Bulk move of all dt-fission-m2-reserve bugs to Fission MVP milestone.

Comment 3

[Chris Peterson [:cpeterson]]

Moving old "dt-fission-m2-reserve" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 4

[Alexandre Poirot [:ochameau]]

The browser toolbox is the last usecase for Legacy Worker Listeners.
This is very complex and possibly brittle code. This is also likely slow.

Bug 1883847 will implement the last missing worker support on the server side (shared worker).
Once this is implemented, we should be able to migrate all worker target type to be listened directly from the server.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Enable server side worker targets for browser toolbox.

Note that the ignore code for subprocess is being ported from the legacy worker watcher.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Ensure waiting for CSS Warning full listening when toggling the filter from tests.

The enhancer was brittle as nothing was waiting for its completion.
We should rather do such work from the action layer. At least this can delay the action completion,
which the test header (setFilterState) already wait for.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] ResourceCommand.watchResources should be protected from mutation made to its array argument.

Modifying the content of the list of resources may suddently start or stop receiving new resources types,
that, without calling (un)watchResources again!

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Ensure spawning DistinctSystemPrincipalLoader.sys.mjs only once.

This is important to spawn this ESM only once between regular and browser toolbox,
and especially between various callsites used by the browser toolbox.
Otherwise we may start loading modules in two distinct system loaders
and may have inconsistancies when using instanceof.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Unregister the JS Process Actor only on watcher actor destruction.

Stop unregistering the JS Process actor when removing the last watched target/resource type.
Instead only unregister it when the Watcher Actor is destroyed.

It prevents some race condition where unwatch request (which aren't awaited for)
unregister the actor late and the next test/next toolbox opening would have its actor
unregistered by late destruction code related to the previously closed toolbox.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Ensure destroying active target actors on toolbox closing.

The cleanup work done from didDestroy was only calling unwatch on the Watcher classes,
which was typically unregistering the platform listeners.
We were missing calling destroyTargetsForWatcher on them which is meant to destroy
the target actors, which can also register platform listeners and be leaking data.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651518 - [devtools] Ensure cleaning up workers which are destroyed during their initilization.

We may unwatch for workers, while the creation of the target actor is still ongoing.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7fb390c3965
[devtools] Ensure waiting for CSS Warning full listening when toggling the filter from tests. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/58e71c27f1fd
[devtools] ResourceCommand.watchResources should be protected from mutation made to its array argument. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/03cf235d2a80
[devtools] Ensure spawning DistinctSystemPrincipalLoader.sys.mjs only once. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b0de4113f8fb
[devtools] Unregister the JS Process Actor only on watcher actor destruction. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/efda5c09b0ea
[devtools] Ensure destroying active target actors on toolbox closing. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b2b138889ad6
[devtools] Ensure cleaning up workers which are destroyed during their initilization. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/4ec70db12f48
[devtools] Enable server side worker targets for browser toolbox. r=devtools-reviewers,nchevobbe

Comment 13

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/f7fb390c3965
https://hg.mozilla.org/mozilla-central/rev/58e71c27f1fd
https://hg.mozilla.org/mozilla-central/rev/03cf235d2a80
https://hg.mozilla.org/mozilla-central/rev/b0de4113f8fb
https://hg.mozilla.org/mozilla-central/rev/efda5c09b0ea
https://hg.mozilla.org/mozilla-central/rev/b2b138889ad6
https://hg.mozilla.org/mozilla-central/rev/4ec70db12f48

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80ae2c7908e4
Backed out 8 changesets (bug 1651518, bug 1883847) for causing damp failures at toolbox/browser-toolbox.js CLOSED TREE

Comment 15

[Cristina Horotan [:chorotan]]

Backed out 8 changesets (Bug 1651518, Bug 1883847) for causing damp failures at toolbox/browser-toolbox.js

Backout: https://hg.mozilla.org/integration/autoland/rev/80ae2c7908e4cc558336a6f6a78097d9c2c9317d

Failure push:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C10%2Cx64%2Cwebrender%2Cshippable%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-windows10-64-shippable-qr%2Fopt-talos-damp-other%2Cdamp-other&revision=4ec70db12f4878d64588b475b31299849fc842c2

Failure log: https://treeherder.mozilla.org/logviewer?job_id=459092764&repo=autoland&lineNumber=1746

Comment 16

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/80ae2c7908e4

Comment 17

[Alexandre Poirot [:ochameau]]

Some nice improvements are expected for the browser toolbox:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=0d6bce985b6b5170a3cfced9d74ede8d5d369014&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=382c935afad2732fc831ba891fa7ddc32e611da7&page=1&showOnlyConfident=1
10% improvement on browser-toolbox.debugger-ready.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d778691464db
[devtools] Ensure waiting for CSS Warning full listening when toggling the filter from tests. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/cf71de92d782
[devtools] ResourceCommand.watchResources should be protected from mutation made to its array argument. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/eaab9de5819d
[devtools] Ensure spawning DistinctSystemPrincipalLoader.sys.mjs only once. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/840a0d9db260
[devtools] Unregister the JS Process Actor only on watcher actor destruction. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8fe0c1a4da7f
[devtools] Ensure destroying active target actors on toolbox closing. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/5337435e8bca
[devtools] Ensure cleaning up workers which are destroyed during their initilization. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/481558275c1d
[devtools] Enable server side worker targets for browser toolbox. r=devtools-reviewers,nchevobbe

Comment 19

[Cristian Tuns]

Backed out for causing dt failures in nsTArray.h

• Backout link
• Push with failures
• Failure Log
• Failure line: SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:43 in Length
  SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:2694:23 in AppendElementInternal<nsTArrayInfallibleAllocator, mozilla::dom::WorkerRunnable *const &>

Comment 20

[Alexandre Poirot [:ochameau]]

The tsan failure should now be fixed thanks to bug 1898108.

Try run with tsan:
https://treeherder.mozilla.org/jobs?repo=try&revision=708d2560f5f59482ba5dfcf7a6680e92cbf8f6bc

Devtools try run:
https://treeherder.mozilla.org/jobs?repo=try&revision=45975c0e970e6a071f32e0858301a3715eb414d8

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a7a532511d9
[devtools] Ensure waiting for CSS Warning full listening when toggling the filter from tests. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0b2581daae2b
[devtools] ResourceCommand.watchResources should be protected from mutation made to its array argument. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2daeb7355923
[devtools] Ensure spawning DistinctSystemPrincipalLoader.sys.mjs only once. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/150c38b5301d
[devtools] Unregister the JS Process Actor only on watcher actor destruction. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/983d51157c2b
[devtools] Ensure destroying active target actors on toolbox closing. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/60622119c008
[devtools] Ensure cleaning up workers which are destroyed during their initilization. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2f84db6e81ff
[devtools] Enable server side worker targets for browser toolbox. r=devtools-reviewers,nchevobbe

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0a7a532511d9
https://hg.mozilla.org/mozilla-central/rev/0b2581daae2b
https://hg.mozilla.org/mozilla-central/rev/2daeb7355923
https://hg.mozilla.org/mozilla-central/rev/150c38b5301d
https://hg.mozilla.org/mozilla-central/rev/983d51157c2b
https://hg.mozilla.org/mozilla-central/rev/60622119c008
https://hg.mozilla.org/mozilla-central/rev/2f84db6e81ff

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Perfherder has detected a devtools performance change from push 7ae12e7835d17b4543af624e6562b55edc977568.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
144%	damp simple.styleeditor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	4.03 -> 9.84
142%	damp simple.styleeditor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	4.14 -> 10.02
80%	damp simple.inspector.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	6.65 -> 11.99
79%	damp simple.inspector.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	6.46 -> 11.58
75%	damp simple.netmonitor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	6.72 -> 11.75
69%	damp simple.netmonitor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	7.01 -> 11.83
58%	damp simple.webconsole.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	7.92 -> 12.54
56%	damp simple.webconsole.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	8.14 -> 12.70
46%	damp custom.styleeditor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	9.26 -> 13.48
45%	damp custom.styleeditor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	9.40 -> 13.61
...	...	...	...	...
3%	damp console.log-in-loop-content-process-bigint	macosx1015-64-shippable-qr	e10s fission stylo webrender	31.47 -> 32.48
3%	damp console.log-in-loop-content-process-bigint	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	31.42 -> 32.36
3%	damp console.log-in-loop-content-process-bool	macosx1015-64-shippable-qr	e10s fission stylo webrender	29.21 -> 30.08
3%	damp console.log-in-loop-content-process-bool	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	29.28 -> 30.06
2%	damp console.log-in-loop-content-process-typedarray	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	60.77 -> 61.99

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
18%	damp browser-toolbox.connect.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	1,871.78 -> 1,530.36
17%	damp browser-toolbox.connect.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1,824.66 -> 1,510.04
17%	damp console.log-in-loop-content-process-promise	macosx1015-64-shippable-qr	e10s fission stylo webrender	61.14 -> 50.69
16%	damp console.log-in-loop-content-process-promise	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	61.10 -> 51.07
12%	damp browser-toolbox.debugger-ready.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1,056.98 -> 934.64
...	...	...	...	...
2%	damp console.log-in-loop-content-process-infinity	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	31.04 -> 30.29

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 503

For more information on performance sheriffing please see our FAQ.