1767346 - Fenix content process priority is never lowered when creating new tabs

Status: RESOLVED FIXED

(GeckoView Graveyard :: Sandboxing, defect, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Christian reported that the browser becomes unsable when opening a lot of new tabs. I looked into it a little bit and it looks like the process priority is never lowered when opening new tabs, this seems to be a bug around SetPriorityHint.

To reproduce:

• Open new tab from the home screen in fenix (e.g. wikipedia.org)
• Tap on Home button on the url bar
• Open a new tab

Expected:

only one content process is high priority

Actual:

observe /proc/<content process id>/oom_score and notice that the score is low for both content processes

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

OK so the problem is that we're not actually setting mRenderLayers to false ever because of this line: https://searchfox.org/mozilla-central/rev/87ecd21d3ca517f8d90e49b32bf042a754ed8f18/dom/ipc/BrowserParent.cpp#3441-3447 since mIsPreservingLayers is always true for GeckoView.

Emilio, Mike, do you have opinions about letting renderLayers be false even when isPreservingLayers is true? or maybe we should change the name of renderLayers, the way the process manager uses it, it seems to be more like "is this tab visible" rather then "are we actually painting layers for this tab".

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I'm confused, didn't such thing cause the tab previews not to work in the past? I might be misremembering.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

I'm confused, didn't such thing cause the tab previews not to work in the past? I might be misremembering.

I'm sure that's possibile. Perhaps we could make renderLayers read-only and have a separate property that can be read/written to, say, .active such that

renderLayers = active || isPreservingLayers;

?

Comment 4

[Emilio Cobos Álvarez [:emilio]]

But that property already exists and is .docshellIsActive, right?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

What kind of extra work are you seeing when in the background, are we not throttling the refresh driver and so on? Reading this we should be throttling the refresh driver and so on like we do on desktop.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Sorry I think there is a little bit of confusion here. This bug is not about doing too much work when in background, it's just specifically about the priority of background processes being incorrect.

The only content processes that should get high priority are the one that contain the current tab. We have a bug right now where we sometimes don't lower the priority of the background tabs processes on Android.

The process manager uses renderLayers and priorityHint to determine when a tab should be high priority: https://searchfox.org/mozilla-central/source/dom/ipc/BrowserHost.cpp#117-129

The problem is, when priorityHint is set, we try to read renderLayers to see if we should set the process to high priority, but if isPreservingLayers is true, renderLayers always return true, which means that we will never lower the priority of the process (even though we should): https://searchfox.org/mozilla-central/source/dom/ipc/BrowserHost.cpp#144-154

But that property already exists and is .docshellIsActive, right?

I see, so what you're suggesting is that instead of reading renderLayers we read docshellIsActive? is there an easy way in BrowserHost to know when active changes? It would be weird if we used the setter of renderLayers to know when active changes (or read renderLayers in the setter and active in the priorityHint setter).

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1767346 - Use browsing context activeness rather than renderLayers to determine process priority. r=mccr8

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Yes, something like what I attached should do.

Comment 9

[Andrew McCreight [:mccr8]]

Does this mean we're not running browser_ProcessPriorityManager.js on Android? Or is it not testing the right thing?

Comment 10

[Emilio Cobos Álvarez [:emilio]]

So apparently right now it is only run on windows: https://searchfox.org/mozilla-central/rev/997a56b018662e2940c99bbaf57a6ac9d1aa5422/dom/ipc/tests/browser.ini#12

But anyways I don't think browser tests run on Android (Geckoview doesn't have a TabBrowser and so on).

Comment 11

[Andrew McCreight [:mccr8]]

Ah, ok. Well, it would be good to have even a basic test, which would have caught this kind of issue.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

I hope we have tests for activeness (bc activeness is reflected in document.visibilityState and so). And my extension to that test should cover this, android basically has https://searchfox.org/mozilla-central/rev/997a56b018662e2940c99bbaf57a6ac9d1aa5422/mobile/android/chrome/geckoview/geckoview.js#86

But yeah, agi do you know where could I extend a test for Android background tabs or so?

Comment 13

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1767346 - Add process priority test for Android.

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Andrew McCreight [:mccr8] from comment #9)

Does this mean we're not running browser_ProcessPriorityManager.js on Android? Or is it not testing the right thing?

The problem was that the test wasn't testing this specific case (preserveLayers = true). But also yes we don't run browser tests on Android.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

I hope we have tests for activeness (bc activeness is reflected in document.visibilityState and so). And my extension to that test should cover this, android basically has https://searchfox.org/mozilla-central/rev/997a56b018662e2940c99bbaf57a6ac9d1aa5422/mobile/android/chrome/geckoview/geckoview.js#86

But yeah, agi do you know where could I extend a test for Android background tabs or so?

We thought we couldn't write one because the priority was always high during tests, turns out it's a quirk of our process priority code which always starts out as FOREGROUND to prevent killing processes too early after creation (before Gecko has the chance to raise the priority)

I attached a patch that has a test that currently fails but should pass after your patch, extensionCurrentTabRaisesPriority.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b715c304c65
Use browsing context activeness rather than renderLayers to determine process priority. r=mccr8,mconley

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7b715c304c65

Comment 17

[Chris Peterson [:cpeterson]]

Agi says we don't need to uplift this fix to Beta 101.

Comment 18

[[ex-Mozilla] Agi Sferro | :agi]

This actually broke priorityHint even further, so I'm gonna ask the sheriffs to revert.

To reproduce in Fenix:

• Open Fenix and load a web page, verify that /proc/<pid>/oom_score is below 100 for the tab:XX process (can check the PID with ps -A | grep org.mozilla)
• Background Fenix -> oom_score should still be below 100, in my testing it's around 770

reverting the patch fixes this.

I'll separately land a test that will cover this case.

Comment 19

[Sandor Molnar[:smolnar]]

Backed out changeset 7b715c304c65 (bug 1767346) for breaking Android process prioritization.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e6d2fef65b87d2eeeac54c1c86f28a8236f709e3

Comment 20

[Phabricator Automation]

Comment on attachment 9275028 [details]
Bug 1767346 - Add process priority test for Android.

Revision D145476 was moved to bug 1768101. Setting attachment 9275028 [details] to obsolete.

Comment 21

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/e6d2fef65b87

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1767346 - WIP - potential android fix + clean-up

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3a02a659ef
Use browsing context activeness rather than renderLayers to determine process priority. r=mccr8,mconley,geckoview-reviewers,agi

Comment 24

[Marian-Vasile Laza]

Backed out changeset 7b3a02a659ef (Bug 1767346) for causing geckoview failures.
Backout link
Push with failures <--> gv-junit-fis
Failure Log

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72f023837df4
Use browsing context activeness rather than renderLayers to determine process priority. r=mccr8,mconley,geckoview-reviewers,agi

Comment 26

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/72f023837df4

Comment 27

[Chris Peterson [:cpeterson]]

Moving content process management bugs to the new GeckoView::Sandboxing component.