Closed Bug 1704545 Opened 4 years ago Closed 4 years ago

Deactivate all BrowserParents in the priority manager

Categories

(Core :: DOM: Content Processes, task, P2)

task

Tracking

()

RESOLVED FIXED
89 Branch
Fission Milestone M7a
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.

(I have patches for this but I'm having trouble uploading them to Phabricator at the moment.)

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.

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.

Priority: -- → P2

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.

Attachment #9215157 - Attachment description: Bug 1704545, part 2 - Tell the priority manager when a browser parent is deactivated. → Bug 1704545, part 3 - Tell the priority manager when a browser parent is deactivated.
Status: NEW → ASSIGNED
Fission Milestone: --- → M7a
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c070aae7b48 part 1 - Make the priority manager's OnRemoteBrowserFrameShown into a direct call. r=gsvelto https://hg.mozilla.org/integration/autoland/rev/e3aef55a8205 part 2 - Rename TabActivityChanged to ActivityChanged. r=gsvelto https://hg.mozilla.org/integration/autoland/rev/b29fdf0da851 part 3 - Tell the priority manager when a browser parent is deactivated. r=gsvelto,kmag
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Regressions: 1705181
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: