1704545 - Deactivate all BrowserParents in the priority manager

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1704545, part 1 - Make the priority manager's OnRemoteBrowserFrameShown into a direct call.

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.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1704545, part 3 - Tell the priority manager when a browser parent is deactivated.

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1704545, part 2 - Rename TabActivityChanged to ActivityChanged.

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3c070aae7b48
https://hg.mozilla.org/mozilla-central/rev/e3aef55a8205
https://hg.mozilla.org/mozilla-central/rev/b29fdf0da851