1618547 - Make process priority manager adjust priorities when frames are in the foreground, as opposed to tabs

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

The process priority manager is very tab-centric, and will downgrade the OS process priority of subframes if they don't have an associated top-level frame in the foreground.

We should make it so that the process priority manager cares about which frames are in the foreground, rather than which tabs.

Comment 1

[Chris Peterson [:cpeterson]]

M6 because we should make sure the process priority manager is not making things worse for Fission before we enable Fission for Nightly users.

P3 because not urgent.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission M7 Beta or later unless we know this bug is causing priority problems.

Comment 3

[Neha Kochar [:neha]]

Randell will find out what's left to do here.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

The setup around the priority manager seems a bit odd right now. It doesn't look like we'll ever interact with this value or the priority manager except through frontend JS either calling browser.deprioritize() on the old browser, or setting browser.renderLayers. Neither of these appears to ever be called on non-toplevel BrowsingContext instances.

I think that getting a passible implementation of this working may be as simple as iterating over the tree of BrowserParent instances in ProcessPriorityManagerIMpl::TabActivityChanged, and performing the logic for each of these frames, rather than only the toplevel one.

This may also eventually want some integration with Fission BFCache in order to make sure we deprioritize processes for tabs in the cache, but given we don't have an implementation of that, we can probably do it later.

Comment 5

[Neha Kochar [:neha]]

Load balancing from pbone to mccr8. Thanks Andrew for picking this one up for M7.

Comment 6

[Chris Peterson [:cpeterson]]

Priority P1 for Fission M7

Comment 7

[Neha Kochar [:neha]]

We'll fix this in M7a.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

We were talking about this just yesterday. In the context of Fission this needs to be addressed or we might have some really odd behavior.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Gabriele Svelto [:gsvelto] from comment #8)

We were talking about this just yesterday. In the context of Fission this needs to be addressed or we might have some really odd behavior.

I've been working on it. There's just a lot going on here.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Cool, let me know if you need any help.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1618547 - Support Fission in the process priority manager.

With Fission, there can be multiple BrowserParents in a single tab, so
this patch moves the tracking of active tabs onto the top BrowsingContext
in a tab. If the priority of a top BC is changed, then the activity
of all of the BPs of the BCs in the tree are all adjusted.

In addition, if a BrowserParent is being newly created, and it is
in an active BC tree, then the priority manager is told to mark the
BP as active. This is needed for the case of an out-of-process iframe
being loaded into a foreground tab.

A prior patch fixed the case of a BrowserParent being deprioritized.

Comment 12

[Andrew McCreight [:mccr8]]

Here's my WIP patch. I think it deals with all of the scenarios I've thought of, but I need to confirm that by writing more tests.

Basic test scenarios:

• When tab with iframes starts getting painted, all processes become active. (BrowserHost::SetRenderLayers)
• When a tab with iframes moves into the background, all processes become inactive. (BrowserHost::Deprioritize)
• BrowserParent::Deactivated() causes the browser parent to get deactivated (bug 1704545, part 3)
• When an active CBC navigates, the new CBC is active. (CanonicalBrowsingContext::ReplacedBy())
• When a BrowserParent is loaded into an active BC tree, it becomes active. (BrowserParent::BrowserParent)

The first two are covered by a modified version of an existing test, but I still need to do the others.

Comment 13

[Andrew McCreight [:mccr8]]

Locally, I have modified the priority manager's testing framework to look at the entire BC tree and not just the browser.

I think the third and fifth scenarios can be tested by navigating an iframe in a foreground tab, with unique origins. With Fission, the original iframe's process should get deprioritized. With and without Fission, the new iframe's process should be prioritized.

I think the fourth scenario can be tested by navigating the top level frame in a way that will require a BC change. I think that won't engage the tab switching logic, so it will rely on CanonicalBrowsingContext::ReplacedBy() and BrowserParent::BrowserParent dealing with the load correctly.

Comment 14

[Andrew McCreight [:mccr8]]

I've been writing tests. My patch here has a bug that BrowserHost::SetRenderLayers was always prioritizing, but it should be passing in aRenderLayers instead of true.

Comment 15

[Andrew McCreight [:mccr8]]

I think my patch is ready for review. There are two failures (a leak while running accessible/tests/browser/e10s/browser_treeupdate_removal.js
and an assertion failure while running toolkit/components/pdfjs/test/browser_pdfjs_saveas.js) but we apparently don't run debug Fission BC tests on Windows, so I'm going to assume those are just existing issues.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f45996fe15b2
Support Fission in the process priority manager. r=kmag,gsvelto

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out changeset f45996fe15b2 (Bug 1618547) for causing failures in browser_ProcessPriorityManager.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=XgdpJUT_R0eW0c6VLTRfxQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f45996fe15b23696c88f85a6b154146d3717fc58

Log: https://treeherder.mozilla.org/logviewer?job_id=338864217&repo=autoland&lineNumber=7003

Backout: https://hg.mozilla.org/integration/autoland/rev/6e4f23fc5d85b490425acce58c80342f46eb0327

Comment 18

[Andrew McCreight [:mccr8]]

Hmm, that's strange. I pushed to try before, and tested locally before I landed, but maybe I did something wrong while addressing review comments that my local testing didn't catch.

Comment 19

[Andrew McCreight [:mccr8]]

I've had a chance to dig into this a bit more. Funnily enough, the failures in browser_ProcessPriorityManager.js appear to have been broken by bug 1693250, which landed immediately before my patch, and was backed out soon after. I confirmed this locally by backing out that other patch while keeping my own. So I guess I'll just reland.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07c8392cb04d
Support Fission in the process priority manager. r=kmag,gsvelto

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/07c8392cb04d