Merge all JS Window/Process Actors specific per target type into a unique DevTools JS Process Actor in the watcher architecture
Categories
(DevTools :: Framework, task)
Tracking
(firefox126 fixed)
Tracking | Status | |
---|---|---|
firefox126 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
In bug 1648499, bug 1651522, bug 1593937 and bug 1633712, lots of code has been duplicated to communicate with each target types (window global, workers, service workers, content processes).
This introduce some maintenance burden as quite some duplicated code.
Also, this may start being a performance bottleneck as we we have to pass a new IPC message / JS Actor message/query per target. This may send duplicated messages to the same content process as soon as we have many target in the same content process, which is very common. Ideally we should send messages once per content process.
Assignee | ||
Comment 1•7 months ago
|
||
-
The formers JSWindow Actors used independently to create Target Actor for WindowGlobals, Web workers and Service workers
are all unified behind a single JSProcess Actor pair which was only used to create target actors for DOM Content Processes. -
The DevToolsProcess JSProcess actor now actively monitor the currently watched target types
in order to start and stop listeners specific to each target type.
We no longer rely on JSWindow Actors to observe WindowGlobal instantiations. -
watchedByDevTools
is now consistantly set from each WindowGlobal's content process, via the WindowGlobalTargetActor.
With the new setup, the parent process no longer track WindowGlobal/BrowsingContext's and so there is no
natural place to flag them. While, in the content process, the target actor is an obvious place.
There is just one trick in window global target watcher in order to also set this flag on initial about blank documents. -
browser-element-host
Session Data is now slightly more simple. It works like any other similar session data attribute.
But we have to ignore any pending exception coming up from updateDomainSessionDataForServiceWorkers call as that be still pending
while the toolbox closes, which make the JS Process Actor be unregistered,
and ultimately make the underlying sendQuery promise be rejected. -
The
loader
is made specific to each Watcher/connection and put in the connections map,
so that we can better support content toolbox & browser toolbox usecases
and we have a unique place to define the loader per watcher/connection.
Assignee | ||
Comment 2•6 months ago
|
||
The test wasn't selected the newly created tab and was trying to open devtools
on the blank tab.
Assignee | ||
Comment 3•6 months ago
|
||
This patch queue has a significant impact on many close subtests:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=194d57f254ac1a32c7ef256045b095c6572d8e96&originalSignature=4943452&newSignature=4943452&framework=12&application=firefox&originalRevision=c6a4531677ff6613d22bcc69d8abc681d6fd0092&page=1&showOnlyConfident=1
This is a bit disappointing, I was expecting more improvement on other subtests.
Assignee | ||
Comment 4•6 months ago
|
||
This tweak helps run the test many times in a row with --run-until-failure.
Assignee | ||
Comment 5•6 months ago
|
||
Assignee | ||
Comment 6•6 months ago
|
||
New DAMP run with all panels tests running. Similar results, mostly improve close tests:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=a2ce65df305aae85c785ca6370e9a4f6b9f785ba&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=53262ea1108a637b412534156e588acf09ef53f1&page=1&showOnlyConfident=1
Small regression appear on cold test. Some modules may be worth lazy loading.
Updated•6 months ago
|
Assignee | ||
Comment 7•6 months ago
|
||
Assignee | ||
Updated•6 months ago
|
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9bba11cbefa7 [devtools] Test against the right tab in browser_command_line_urls.js. r=devtools-reviewers,nchevobbe https://hg.mozilla.org/integration/autoland/rev/041a44007fb4 [devtools] Wait for the location to be updated in browser_command_line_urls.js. r=devtools-reviewers,nchevobbe
Comment 9•6 months ago
|
||
bugherder |
Comment 10•6 months ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d27f783be149 [devtools] Merge all DevTools JS Actors into a single DOM Process JS Actor. r=devtools-reviewers,nchevobbe https://hg.mozilla.org/integration/autoland/rev/f503f5d02d1e [devtools] Rename WatcherRegistry to ParentProcessWatcherRegistry. r=devtools-reviewers,nchevobbe https://hg.mozilla.org/integration/autoland/rev/5d59dd808307 [devtools] Document the new DevTools backend usage of JS Process Actors. r=devtools-reviewers,bomsy
Comment 11•6 months ago
•
|
||
Backed out for causing bc failures in browser_ext_devtools_inspectedWindow_targetSwitch.js
- Backout link
- Push with failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_targetSwitch.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_targetSwitch.js | Uncaught exception received from previously timed out test bound - at resource://testing-common/BrowserTestUtils.sys.mjs:538 - Error: The window unloaded while we were waiting for the browser to load - this should never happen.
Assignee | ||
Comment 12•6 months ago
|
||
The WebExtension mochitest failure should be fixed now.
Comment 13•6 months ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a3d3dd0da81d [devtools] Merge all DevTools JS Actors into a single DOM Process JS Actor. r=devtools-reviewers,nchevobbe https://hg.mozilla.org/integration/autoland/rev/2c19eeb928f3 [devtools] Rename WatcherRegistry to ParentProcessWatcherRegistry. r=devtools-reviewers,nchevobbe https://hg.mozilla.org/integration/autoland/rev/52bb75377d06 [devtools] Document the new DevTools backend usage of JS Process Actors. r=devtools-reviewers,bomsy
Comment 14•6 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3d3dd0da81d
https://hg.mozilla.org/mozilla-central/rev/2c19eeb928f3
https://hg.mozilla.org/mozilla-central/rev/52bb75377d06
Comment 15•6 months ago
|
||
== Change summary for alert #42098 (as of Tue, 02 Apr 2024 12:55:03 GMT) ==
Regressions:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
41% | reload-debugger:content-process objects-with-stacks | linux1804-64-qr | 986.00 -> 1,394.00 | |
39% | reload-webconsole:content-process objects-with-stacks | linux1804-64-qr | 1,001.00 -> 1,396.00 | |
39% | reload-webconsole:content-process objects-with-stacks | linux1804-64-shippable-qr | 1,001.00 -> 1,396.00 | |
39% | reload-netmonitor:content-process objects-with-stacks | linux1804-64-qr | 1,006.00 -> 1,400.00 | |
26% | reload-debugger:content-process memory | linux1804-64-qr | 2,716,672.00 -> 3,417,088.00 | |
10% | reload-inspector:content-process objects-with-stacks | linux1804-64-qr | 4,038.56 -> 4,430.50 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=42098
Description
•