Create tab targets on process change via the Watcher Actor
Categories
(DevTools :: Framework, task)
Tracking
(Fission Milestone:M7a, firefox89 fixed)
Tracking | Status | |
---|---|---|
firefox89 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
(Whiteboard: dt-fission-m3-mvp)
Attachments
(4 files, 4 obsolete files)
Bug 1644360 highlighted that server side resource listening is broken for top level document because its target is lazily created. Because it is lazily created, from the frontend by LocalTargetTarget.onRemotenessChange, we start watching for resources late.
In order to fix that, we should create the tab target from the server side, by the Watcher actor and its helpers. So that we create the target before the page starts loading.
We don't have to change how we create the first target, but we have to handle all targets that may be created when navigating to another origin while Fission is enabled.
The first target can still be created via TabDescriptor.getTarget
, but target switching should probably be done server side.
Such work relates to bug 1618693. If we move target switching to the server, the LocalTabTargetFront may become almost empty and be removed.
The unanswered challenges are:
- can we remove the LocalTabTargetFront code related to target switching? Could the new server side option be breaking many things, and we would have to make the new mode be yet another option?
- will it be easy to create a message manager target, surviving reload? Or should we switch to JS Window Actor target actors when we navigate to a new origin with Fission? This would break quite significantly reloads after such navigation.
Supporting early resource on cross origin navigation sounds like an important use case? So flagging this MVP.
Comment 1•4 years ago
|
||
Tracking dt-fission-m2 bugs for Fission Nightly (M6c)
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Bulk move of all m2-mvp devtools bugs to Fission M7.
Updated•4 years ago
|
Comment 3•4 years ago
|
||
This work is currently blocked on removing all call sites for setupInParent (and other mm-based APIs) in devtools actors.
Comment 4•4 years ago
|
||
Some relevant try pushes:
- adding setupInParent support to JSWindowActor (workaround): https://treeherder.mozilla.org/jobs?repo=try&revision=72ae806873f96c74ad96606413c8e72b5a084cdb
- WIP to create target from the watcher: https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=f161d123347b2c12e9d76de460d8141a52d0cfda
Assignee | ||
Comment 5•4 years ago
|
||
Once bug 1665383 is fixed, the problematic spawnActorInParentProcess
call should no longer be used in production:
https://searchfox.org/mozilla-central/rev/07342ce09126c513540c1c343476e026cfa907bf/devtools/server/actors/webconsole.js#681
But it will still be in all tests which are still querying WebConsoleFront.startListeners["NetworkActivity"]
, bug 1686723 covers refactoring these network tests.
There there is the problematic setupInParent
calls from storage actor:
https://searchfox.org/mozilla-central/search?q=setupInParent%28&path=&case=false®exp=false
Similarly to network monitor, once bug 1644192, only tests which are still querying the storage actor directly, without passing through the ResourceWatcher will be using setupInParent
. When we go through the ResourceWatcher, we instantiate a special instance of StorageActor, which avoids trigerring the codepaths involving setupInParent. Bug 1686727 covers refactoring these storage tests.
If we want to land this bug before bug 1665383 and bug 1644192, we have to be careful as it would break network and storage on navigations/reloads. So we would better use a preference for landing this.
We might reuse the devtools.testing.enableServerWatcherSupport preference. But we may as well use a dedicated preference.
Turning this on by default would force us to refactor tests and fix bug 1686723 and bug 1686727.
Finally, once this bug gets its main patch landed, we can probably revisit the code in ResourceWatcher.onTargetAvailable:
https://searchfox.org/mozilla-central/rev/2a24205479519e70c0574929f45730d285141584/devtools/shared/resources/resource-watcher.js#264-281
In order to stop reseting/restoring resource watching on top level target switching.
This is what causes bug 1601331 to fail and probably many other tests.
And last but not least, could it be that bug 1602748 ends up being a duplicate of this one?
Updated•4 years ago
|
Comment 7•4 years ago
•
|
||
There is an interesting issue with the resource-watcher and top level target switching. I have mostly seen the issue with document event resources, which could arguably be replaced with targetDestroyed/Available once all targets are going through proper target switching. But it feels like the pattern could be an issue with any resource.
My scenario, I navigate from a parent process page to a content process page.
The style editor is opened and watches for document event resources.
As soon as the content process page is created on the server, the target is created and we start watching for document events.
We receive the target on the client side, but before we can notify the resource watcher, we have to attach the thread.
In the meantime, we emit the dom-loading, dom-interactive, dom-complete events.
Since we are not yet listening for resources on this target on the client side, they end up in the resource cache of the targetFront (searchfox).
Then the thread attaches and we finally call the target-available callbacks, including resourceWatcher onTargetAvailable.
Since this is a target-switching, we call stopListening
for all resources.
Then we call targetFront.on("resource-available-form",...)
(searchfox).
This will release our cached document-events.
Finally we call startListening
for all resources, including document events. This means we recreate the server-side listener. Since the document is already loaded, we will recreate the document events and emit them a second time.
Comment 8•4 years ago
|
||
The target-front cache was introduced for a similar situation, but for a resource which would not be re-emitted: https://searchfox.org/mozilla-central/rev/f27594d62e7f1d57626889255ce6a3071d67209f/devtools/server/actors/resources/stylesheets.js#520-544
Maybe we should differentiate resources that will be safely recreated when the listeners restart (document events, maybe console messages?) from the ones that are not.
Comment 9•3 years ago
|
||
Comment 10•3 years ago
|
||
Depends on D105540
Comment 11•3 years ago
|
||
Depends on D105542
Comment 12•3 years ago
|
||
Depends on D105544
Comment 13•3 years ago
|
||
Depends on D105545
Comment 14•3 years ago
|
||
Depends on D105546
Comment 15•3 years ago
|
||
Tracking for Fission M8 milestone (Release experiment).
Comment 16•3 years ago
|
||
Comment on attachment 9203836 [details]
Bug 1644397 - [devtools] Read isTopLevelTarget server flag to trigger target switching
Revision D105540 was moved to bug 1697118. Setting attachment 9203836 [details] to obsolete.
Comment 17•3 years ago
|
||
Comment on attachment 9203839 [details]
Bug 1644397 - [devtools] Check followWindowGlobalLifeCycle flag in navigateTo test helper
Revision D105542 was moved to bug 1697118. Setting attachment 9203839 [details] to obsolete.
Comment 18•3 years ago
|
||
Nicolas agreed to take over this bug, thanks!
Assignee | ||
Comment 19•3 years ago
|
||
Picking up in coordination with Nicolas.
I rebased the patches on top of my destroy tweaks.
Assignee | ||
Comment 20•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 21•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 22•3 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/44e93e1b6599 [devtools] Create tab targets on process change via the Watcher Actor. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/78f802bd8a25 [devtools] Check if we should unregisterJSWindowActors after destroying a WatcherActor. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/de158755e6d4 [devtools] Add dedicated navigation test. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/b54e509090c5 [devtools] Add test for duplicated early messages in console. r=nchevobbe
Comment 23•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44e93e1b6599
https://hg.mozilla.org/mozilla-central/rev/78f802bd8a25
https://hg.mozilla.org/mozilla-central/rev/de158755e6d4
https://hg.mozilla.org/mozilla-central/rev/b54e509090c5
Description
•