1931062 - [Fission] Investigate setActive and if it needs to be adjusted for Fission

Status: RESOLVED FIXED

(GeckoView :: General, task, P2)

Description

[Olivia Hall [:olivia]]

In GeckoViewTest, while working on bug 1768102, it was discovered that when fission is on, the way the API setActive works may need some additional investigation.

A new test setActiveProcessPriorityTest was added there to illustrate the differences. That test is only there to show the behavior - it doesn't make an assumption that the current behavior is correct.

Issue summary:

• Set active is set to false
• Session goes to BACKGROUND and docshell activeness is false
• Load a new URI
• New PID is made
• That new PID has FOREGROUND priority and docshell activeness is false

Investigation is if this behavior is as expected or something we need to monitor.

Comment 1

[Olivia Hall [:olivia]]

I'm not sure if I'd consider this blocking bug 1610822 (fission), but it'd be nice to know if this behavior is okay / expected or if it requires adjustments. I'm setting it to block for now.

Part of the investigation should be seeing if this can happen outside of a test case in Fenix through starting a load and immediately switching tabs.

Comment 2

[Olivia Hall [:olivia]]

I don't think it is related, but I had some deja vu with bug 1847566, which might have some ideas for testing in Fenix and thought it might be worth mentioning.

Comment 3

[Olivia Hall [:olivia]]

Hi Nika,

When I was fixing bug 1768102, I saw something around process priority and the setActive API call that I wanted to check-in with you about. I don't know what the behavior should be or if this is just a slight nuiance change post-fission.

• Without fission enabled, we had a test that worked by open a session -> setActive(false) -> load URI -> check for low-priority BACKGROUND PID.
• It stopped working with fission enabled because the checked PID wasn't BACKGROUND. I fixed it by open a session -> load URI -> setActive(false) -> check for low-priority BACKGROUND PID.
  • It originally stopped working because the new process switched PID after loading was FOREGROUND priority.

The question I have is if setActive(false) should always mean the session is low priority as before fission or if it is okay/expected that it gains FOREGROUND priority after the load.

• This comment illustrates what I was seeing. (There are more logs on that patch if that helps.)
• I wrote this test that tries to illustrate the behavior.
• Here is where the setActive calls originated from.

I'm also going to test some with Fenix to check if this scenario can happen organically outside of a test too.

Comment 4

[Andrew McCreight [:mccr8]]

I wasn't thinking about what might happen if a low-priority process did a process switch navigation when I was making the priority manager work on Fission desktop, so there might be some issues there. I'm not sure what exactly the behavior should be, but having it sit at high priority forever doesn't sound great.

Comment 5

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

The activeness/renderLayers/processPriority/etc. logic is unfortunately quite difficult to follow, and has a bunch of somewhat-independent-yet-interconnected flags. We probably should unify these different code paths at some point into something more consistent with a clear source of truth, but that's probably out of scope for this issue.

Looking at the various places which set the priority level, they're all somewhat inconsistent, but I don't see a specific place which seems like it would lead to this specific issue after reading. I found a few places where I thought things might misbehave, but it appears they should work OK in this scenario.

I wonder a bit whether the oom_score value being read in the test is actually process-specific and not a direct reflection of the priority levels which we set in Gecko. I'm not sure if this is likely to be true, but it might be worth looking at the calls to https://searchfox.org/mozilla-central/rev/6050bf4eca89956c9d91bfd89fa59294ae32a689/hal/android/AndroidProcessPriority.cpp#34 or the corresponding Java method (https://searchfox.org/mozilla-central/rev/6050bf4eca89956c9d91bfd89fa59294ae32a689/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/ServiceAllocator.java#171-174,180-193) to see what values are being set for the new process after the process switch.

Comment 6

[Olivia Hall [:olivia]]

Attachment: Diff-4-logging-11-06-2024.md

Search "Raw Full log" to avoid my commentary. This was from the patch on bug 1768102 with the test oom scores doing just a direct comparison. (Otherwise conditions before the test change.)

Comment 7

[Olivia Hall [:olivia]]

Thanks, Nika!

I wonder a bit whether the oom_score value being read in the test is actually process-specific and not a direct reflection of the priority levels which we set in Gecko.

For sure, the oom_score in the test is process-specific (we were seeing much more varied values when swiching emulators even).

I mostly came to this conclusion through process priority logging rather than looking at oom scores.

I had this at the bottom of AndroidProcessPriority.cpp SetProcessPriority while testing:

  const char* geckoPriorityString = ProcessPriorityToString(aPriority);
  const char* javaPriorityString = "UNKNOWN";
  if (priorityLevel == mozilla::java::ServiceAllocator::PriorityLevel::FOREGROUND()) {
    javaPriorityString = "FOREGROUND";
  } else if (priorityLevel == mozilla::java::ServiceAllocator::PriorityLevel::BACKGROUND()) {
    javaPriorityString = "BACKGROUND";
  } else if (priorityLevel == mozilla::java::ServiceAllocator::PriorityLevel::IDLE()) {
    javaPriorityString = "IDLE";
  }

  __android_log_print(ANDROID_LOG_INFO, "GV - SetProcessPriority ",
                      "PID: %d, C++ Priority: %s (%d), Java Priority: %s (%p), Relative Importance: %d",
                      aPid, geckoPriorityString, intPriority, javaPriorityString, priorityLevel.Get(), relativeImportance);

I was having to infer a bit that a process switch occured.

I'll take some more logs while manually testing since the other logging was particularly specific for the test scenario and might not be useful for a general discussion.

Comment 8

[Olivia Hall [:olivia]]

Attachment: setActiveProcessPriorityTest-with-fission-2024-11-19.md

Logs on 11/19/2024 for setActiveProcessPriorityTest using ./mach geckoview-junit org.mozilla.geckoview.test.GeckoViewTest#setActiveProcessPriorityTest. (Fission enabled.) Have "Abbreviated Log" and "Full Log" in file.

Comment 9

[Olivia Hall [:olivia]]

Attachment: setActiveProcessPriorityTest-without-fission-2024-11-19.md

Logs on 11/19/2024 for setActiveProcessPriorityTest using ./mach geckoview-junit org.mozilla.geckoview.test.GeckoViewTest#setActiveProcessPriorityTest --disable-fission. (Fission disabled.) Have "Abbreviated Log" and "Full Log" in file.

Comment 10

[Olivia Hall [:olivia]]

Attachment: setActiveProcessPriorityTest-Logging-2024-11-19.diff

Diff to show where logs came from in case I made a mistake or typo and to show log origins.

Comment 11

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

Attachment: Bug 1931062 - Part 1: Always set browser priority in BrowserParent constructor, r=mccr8!

Without this change if we're in a new process, we won't lower the priority when
navigating a background browser into the process. This should ensure we always
re-compute priority after creating a new BrowserParent, even if the
BrowsingContext is not active.

Comment 12

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

Attachment: Bug 1931062 - Part 2: Directly visit BrowserParent for IsPriorityActive, r=smaug!

Previously we were calling these methods more often than necessary, as they'd
be called multiple times per-BrowserParent if a single BrowserParent is hosting
multiple frames.

Comment 13

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

:olivia, would you be able to check and see if these patches fix the issue?

Comment 14

[Olivia Hall [:olivia]]

Attachment: Bug 1931062 - Update GeckoViewTest Priority Expectations

This patch reverts part of bug 1768102 and adjusts the test
setActiveProcessPriorityTest based on the new fission behavior.

Comment 15

[Olivia Hall [:olivia]]

Thank you, Nika!

The changes worked really well for me locally. I compared some logs and it looks in alignment with the pre-fission behavior on Android. I also updated the GV tests in D229653. They fail without your patches and pass with the patches. Part of the test change is reverting the shifting of the setActive in bug 1768102, which was how the issue was uncovered. I sent a try run up for D229653 that I'll attach to the patch too.

Comment 16

[Olivia Hall [:olivia]]

Hi Nika,

I changed the test conditions in D229653 and the trys look more like they did pre-fission with the stack applied.

Just a ni? to see if you would like to go forward with these changes or not. Thanks so much for investigating this bug!

Comment 17

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

(In reply to Olivia Hall [:olivia] from comment #16)

Hi Nika,

I changed the test conditions in D229653 and the trys look more like they did pre-fission with the stack applied.

Just a ni? to see if you would like to go forward with these changes or not. Thanks so much for investigating this bug!

I've pushed new non-WIP versions of the patch stack to phab.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68fd56c9b8d1
Part 1: Always set browser priority in BrowserParent constructor, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6da191cbac0f
Part 2: Directly visit BrowserParent for IsPriorityActive, r=smaug
https://hg.mozilla.org/integration/autoland/rev/b99d4cafe994
Update GeckoViewTest Priority Expectations r=geckoview-reviewers,kaya

Comment 19

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/68fd56c9b8d1
https://hg.mozilla.org/mozilla-central/rev/6da191cbac0f
https://hg.mozilla.org/mozilla-central/rev/b99d4cafe994