Enable all server side worker targets for the browser toolbox
Categories
(DevTools :: Framework, enhancement)
Tracking
(Fission Milestone:Future, firefox128 fixed)
Tracking | Status | |
---|---|---|
firefox128 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 3 open bugs, Regressed 1 open bug)
Details
(Whiteboard: dt-fission-future)
Attachments
(7 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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•4 years ago
|
||
Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c).
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Bulk move of all dt-fission-m2-reserve bugs to Fission MVP milestone.
Comment 3•4 years ago
|
||
Moving old "dt-fission-m2-reserve" bugs to "dt-fission-future" because they don't block Fission MVP.
Assignee | ||
Comment 4•6 months ago
|
||
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.
Assignee | ||
Comment 5•6 months ago
|
||
Note that the ignore code for subprocess is being ported from the legacy worker watcher.
Assignee | ||
Comment 6•6 months ago
|
||
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.
Assignee | ||
Comment 7•6 months ago
|
||
Modifying the content of the list of resources may suddently start or stop receiving new resources types,
that, without calling (un)watchResources again!
Updated•5 months ago
|
Assignee | ||
Comment 8•5 months ago
|
||
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
.
Updated•5 months ago
|
Assignee | ||
Comment 9•4 months ago
|
||
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.
Assignee | ||
Comment 10•4 months ago
|
||
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.
Assignee | ||
Comment 11•4 months ago
|
||
We may unwatch for workers, while the creation of the target actor is still ongoing.
Updated•4 months ago
|
Comment 12•4 months ago
|
||
Pushed by apoirot@mozilla.com: 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•4 months ago
|
||
bugherder |
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•4 months ago
|
||
Backout by chorotan@mozilla.com: 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•4 months ago
|
||
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 log: https://treeherder.mozilla.org/logviewer?job_id=459092764&repo=autoland&lineNumber=1746
Comment 16•4 months ago
|
||
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/80ae2c7908e4
Assignee | ||
Updated•4 months ago
|
Assignee | ||
Comment 17•4 months ago
|
||
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•4 months ago
|
||
Pushed by apoirot@mozilla.com: 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•4 months ago
|
||
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 &>
Assignee | ||
Updated•4 months ago
|
Assignee | ||
Comment 20•4 months ago
|
||
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•4 months ago
|
||
Pushed by apoirot@mozilla.com: 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•4 months ago
|
||
bugherder |
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•3 months ago
|
||
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.
Description
•