Deactivate all BrowserParents in the priority manager
Categories
(Core :: DOM: Content Processes, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox89 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(3 files)
The priority manager tracks the set of active browser parents (via the tab id) in each process. Right now, browser parents that are going away are only removed from this set when BrowserHost::DestroyComplete() gets called, but in the Fission world BrowserParents for iframes aren't owned by a BrowserHost, so they'll be missed.
This is not a problem right now, because non-top level browser parents are currently never prioritized, but for bug 1618547 we're going to need this.
I think a good fix for this is to deprioritize a browser parent in BrowserParent::Deactivated(). This will also handle the case where a browser parent enters the BFCache, which is not currently dealt with.
This fix gets rid of ParticularProcessPriorityManager's observer for ipc:browser-destroyed, so it is a good excuse to get rid of its observer for remote-browser-shown, which lets us make it no longer be an observer at all. It doesn't seem great to make every one of these an observer in the Fission world where we will have a lot more of them, and the observers are a bit messy. I don't entirely understand the remote-browser-shown behavior, which does not apply to Fission, but I think it is simple enough to preserve the existing behavior.
Assignee | ||
Comment 1•4 years ago
|
||
(I have patches for this but I'm having trouble uploading them to Phabricator at the moment.)
Assignee | ||
Comment 2•4 years ago
|
||
ParticularProcessPriorityManager observes remote-browser-shown and recomputes
the priority for the given browser. This patch eliminates the observer call
and instead makes the frame loader do a direct call.
I think doing it this way is a little clearer, and also the next patch will
eliminate the other reason ParticularProcessPriorityManager is an observer,
which will let us not not register something as an observer for every content
process, which I think is also good.
There should be no change in behavior.
Assignee | ||
Comment 3•4 years ago
|
||
The priority manager tracks which browser parents are active in each process,
so it needs to be told when a browser parent is no longer active. This
is currently implemented by observing ipc:browser-destroyed, which happens
in BrowserHost::DestroyComplete().
However, with Fission, not all BrowserParents are held by BrowserHosts. I
don't think this is an issue right now, but if the priority manager is going
to properly prioritize processes due to the presence of active non-top-level
frames, then it needs to also deprioritize them when they go away.
This patch deals with this situation by directly telling the priority
manager that the browser parent is becoming inactive via the existing
TabActivityChanged method, in the BrowserParent::Deactivated() method,
which is called when a browser parent is being destroyed or entering
the BF cache. I think it makes sense in both cases that we no longer
want to prioritize the process that the page is in.
This does mean that we are telling the priority manager about more
ContentParents which is potentially more expensive, but the old code
also ended up doing a hashtable remove in every single
ParticularProcessPriorityManager, whereas the new code only does it
for one, so hopefully it is a net win overall.
(The reason for this is that in ParticularProcessPriorityManager's
OnBrowserParentDestroyed() method the browserHost->GetContentParent()
check will always return null because the browser host nulls out
mRoot immediately before it does ipc:browser-destroyed, so the hash
table remove is never skipped.)
This patch also removes the last thing that ParticularProcessPriorityManager
was observing, so it no longer needs to be an nsIObserver.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
As of the next patch, it won't be used just for indicating the
activity of an entire tab, so let's rename it.
Also alphabetize the order of some forward declarations.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 6•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c070aae7b48
https://hg.mozilla.org/mozilla-central/rev/e3aef55a8205
https://hg.mozilla.org/mozilla-central/rev/b29fdf0da851
Description
•