Bug 1767346 - Use browsing context activeness rather than renderLayer…

…s to determine process priority. r=mccr8,mconley,geckoview-reviewers,agi For desktop this should basically have no impact (maybe impacts tab warming, but if we wanted we could set the priority hint from the tab switcher the same way we set renderLayers), but Fenix always has renderLayers as true, effectively, so we were never de-prioritizing the background tab processes. Differential Revision: https://phabricator.services.mozilla.com/D145351
Thaodan · May 20, 2022 · b6782fe · b6782fe
1 parent 049e223
commit b6782fe
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 69 deletions.
diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp
@@ -21,6 +21,7 @@
 #endif
 #include "mozilla/AppShutdown.h"
 #include "mozilla/dom/CanonicalBrowsingContext.h"
+#include "mozilla/dom/BrowserHost.h"
 #include "mozilla/dom/BrowserChild.h"
 #include "mozilla/dom/BrowserParent.h"
 #include "mozilla/dom/BrowsingContextGroup.h"
@@ -2641,25 +2642,29 @@ void BrowsingContext::DidSet(FieldIndex<IDX_ExplicitActive>,
   if (IsTop()) {
     Group()->UpdateToplevelsSuspendedIfNeeded();
 
+    if (XRE_IsParentProcess()) {
+      auto* bc = Canonical();
+      if (BrowserParent* bp = bc->GetBrowserParent()) {
+        bp->RecomputeProcessPriority();
 #if defined(XP_WIN) && defined(ACCESSIBILITY)
-    if (XRE_IsParentProcess() && a11y::Compatibility::IsDolphin()) {
-      // update active accessible documents on windows
-      if (BrowserParent* bp = Canonical()->GetBrowserParent()) {
-        if (a11y::DocAccessibleParent* tabDoc =
-                bp->GetTopLevelDocAccessible()) {
-          HWND window = tabDoc->GetEmulatedWindowHandle();
-          MOZ_ASSERT(window);
-          if (window) {
-            if (isActive) {
-              a11y::nsWinUtils::ShowNativeWindow(window);
-            } else {
-              a11y::nsWinUtils::HideNativeWindow(window);
+        if (a11y::Compatibility::IsDolphin()) {
+          // update active accessible documents on windows
+          if (a11y::DocAccessibleParent* tabDoc =
+                  bp->GetTopLevelDocAccessible()) {
+            HWND window = tabDoc->GetEmulatedWindowHandle();
+            MOZ_ASSERT(window);
+            if (window) {
+              if (isActive) {
+                a11y::nsWinUtils::ShowNativeWindow(window);
+              } else {
+                a11y::nsWinUtils::HideNativeWindow(window);
+              }
             }
           }
         }
+#endif
       }
     }
-#endif
   }
 
   PreOrderWalk([&](BrowsingContext* aContext) {

diff --git a/dom/ipc/BrowserHost.cpp b/dom/ipc/BrowserHost.cpp
@@ -120,22 +120,14 @@ BrowserHost::SetRenderLayers(bool aRenderLayers) {
     return NS_OK;
   }
 
-  bool priorityHint;
-  GetPriorityHint(&priorityHint);
-  ProcessPriorityManager::BrowserPriorityChanged(
-      GetBrowsingContext()->Canonical(), priorityHint || aRenderLayers);
   mRoot->SetRenderLayers(aRenderLayers);
   return NS_OK;
 }
 
 /* readonly attribute boolean hasLayers; */
 NS_IMETHODIMP
 BrowserHost::GetHasLayers(bool* aHasLayers) {
-  if (!mRoot) {
-    *aHasLayers = false;
-    return NS_OK;
-  }
-  *aHasLayers = mRoot->GetHasLayers();
+  *aHasLayers = mRoot && mRoot->GetHasLayers();
   return NS_OK;
 }
 
@@ -145,27 +137,19 @@ BrowserHost::SetPriorityHint(bool aPriorityHint) {
   if (!mRoot) {
     return NS_OK;
   }
-  bool renderLayers;
-  GetRenderLayers(&renderLayers);
-  ProcessPriorityManager::BrowserPriorityChanged(
-      GetBrowsingContext()->Canonical(), aPriorityHint || renderLayers);
   mRoot->SetPriorityHint(aPriorityHint);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 BrowserHost::GetPriorityHint(bool* aPriorityHint) {
-  if (!mRoot) {
-    *aPriorityHint = false;
-    return NS_OK;
-  }
-  *aPriorityHint = mRoot->GetPriorityHint();
+  *aPriorityHint = mRoot && mRoot->GetPriorityHint();
   return NS_OK;
 }
 
 /* void resolutionChanged (); */
 NS_IMETHODIMP
-BrowserHost::NotifyResolutionChanged(void) {
+BrowserHost::NotifyResolutionChanged() {
   if (!mRoot) {
     return NS_OK;
   }
@@ -177,13 +161,13 @@ BrowserHost::NotifyResolutionChanged(void) {
 
 /* void deprioritize (); */
 NS_IMETHODIMP
-BrowserHost::Deprioritize(void) {
+BrowserHost::Deprioritize() {
   if (!mRoot) {
     return NS_OK;
   }
-  ProcessPriorityManager::BrowserPriorityChanged(
-      GetBrowsingContext()->Canonical(),
-      /* aPriority = */ false);
+  auto* bc = GetBrowsingContext()->Canonical();
+  ProcessPriorityManager::BrowserPriorityChanged(bc,
+                                                 /* aPriority = */ false);
   return NS_OK;
 }
 

diff --git a/dom/ipc/BrowserParent.cpp b/dom/ipc/BrowserParent.cpp
@@ -162,7 +162,6 @@ using namespace mozilla::widget;
 using namespace mozilla::gfx;
 
 using mozilla::LazyLogModule;
-using mozilla::Unused;
 
 extern mozilla::LazyLogModule gSHIPBFCacheLog;
 
@@ -243,10 +242,18 @@ BrowserParent::BrowserParent(ContentParent* aManager, const TabId& aTabId,
   // that some input events are dispatched before PBrowserConstructor.
   mIsReadyToHandleInputEvents = !ContentParent::IsInputEventQueueSupported();
 
+  // Make sure to compute our process priority if needed before the block of
+  // code below. This makes sure the block below prioritizes our process if
+  // needed.
+  if (aBrowsingContext->IsTop()) {
+    RecomputeProcessPriority();
+  }
+
   // If we're in a BC tree that is active with respect to the priority manager,
   // ensure that this new BrowserParent is marked as active. This ensures that
   // the process will be prioritized in a cross-site iframe navigation in an
-  // active tab.
+  // active tab, and also that the process is correctly prioritized if we got
+  // created for a browsing context which was already active.
   if (aBrowsingContext->Top()->IsPriorityActive()) {
     ProcessPriorityManager::BrowserPriorityChanged(this, true);
   }
@@ -3477,6 +3484,13 @@ bool BrowserParent::GetPriorityHint() { return mPriorityHint; }
 
 void BrowserParent::SetPriorityHint(bool aPriorityHint) {
   mPriorityHint = aPriorityHint;
+  RecomputeProcessPriority();
+}
+
+void BrowserParent::RecomputeProcessPriority() {
+  auto* bc = GetBrowsingContext();
+  ProcessPriorityManager::BrowserPriorityChanged(
+      bc, bc->IsActive() || mPriorityHint);
 }
 
 void BrowserParent::PreserveLayers(bool aPreserveLayers) {

diff --git a/dom/ipc/BrowserParent.h b/dom/ipc/BrowserParent.h
@@ -132,6 +132,8 @@ class BrowserParent final : public PBrowserParent,
 
   CanonicalBrowsingContext* GetBrowsingContext() { return mBrowsingContext; }
 
+  void RecomputeProcessPriority();
+
   already_AddRefed<nsILoadContext> GetLoadContext();
 
   Element* GetOwnerElement() const { return mFrameElement; }

diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
@@ -509,10 +509,20 @@ void ProcessPriorityManagerImpl::NotifyProcessPriorityChanged(
   }
 }
 
+static nsCString BCToString(dom::CanonicalBrowsingContext* aBC) {
+  nsCOMPtr<nsIURI> uri = aBC->GetCurrentURI();
+  return nsPrintfCString("id=%" PRIu64 " uri=%s active=%d pactive=%d",
+                         aBC->Id(),
+                         uri ? uri->GetSpecOrDefault().get() : nullptr,
+                         aBC->IsActive(), aBC->IsPriorityActive());
+}
+
 void ProcessPriorityManagerImpl::BrowserPriorityChanged(
     dom::CanonicalBrowsingContext* aBC, bool aPriority) {
   MOZ_ASSERT(aBC->IsTop());
 
+  LOG("BrowserPriorityChanged(%s, %d)\n", BCToString(aBC).get(), aPriority);
+
   bool alreadyActive = aBC->IsPriorityActive();
   if (alreadyActive == aPriority) {
     return;
@@ -525,6 +535,8 @@ void ProcessPriorityManagerImpl::BrowserPriorityChanged(
 
   aBC->PreOrderWalk([&](BrowsingContext* aContext) {
     CanonicalBrowsingContext* canonical = aContext->Canonical();
+    LOG("PreOrderWalk for %p: %p -> %p, %p\n", aBC, canonical,
+        canonical->GetContentParent(), canonical->GetBrowserParent());
     if (ContentParent* cp = canonical->GetContentParent()) {
       if (RefPtr pppm = GetParticularProcessPriorityManager(cp)) {
         if (auto* bp = canonical->GetBrowserParent()) {
@@ -537,6 +549,8 @@ void ProcessPriorityManagerImpl::BrowserPriorityChanged(
 
 void ProcessPriorityManagerImpl::BrowserPriorityChanged(
     BrowserParent* aBrowserParent, bool aPriority) {
+  LOG("BrowserPriorityChanged(bp=%p, %d)\n", aBrowserParent, aPriority);
+
   if (RefPtr pppm =
           GetParticularProcessPriorityManager(aBrowserParent->Manager())) {
     Telemetry::ScalarAdd(
@@ -766,13 +780,14 @@ void ParticularProcessPriorityManager::SetPriorityNow(
     return;
   }
 
+  LOGP("Changing priority from %s to %s (cp=%p).",
+       ProcessPriorityToString(mPriority), ProcessPriorityToString(aPriority),
+       mContentParent);
+
   if (!mContentParent || mPriority == aPriority) {
     return;
   }
 
-  LOGP("Changing priority from %s to %s.", ProcessPriorityToString(mPriority),
-       ProcessPriorityToString(aPriority));
-
   PROFILER_MARKER(
       "Subprocess Priority", OTHER,
       MarkerOptions(MarkerThreadId::MainThread(), MarkerStack::Capture()),

diff --git a/dom/ipc/tests/browser_ProcessPriorityManager.js b/dom/ipc/tests/browser_ProcessPriorityManager.js
@@ -318,6 +318,19 @@ add_task(async function test_normal_background_tab() {
         "PriorityHint of the original tab should be false on default"
       );
 
+      // Changing renderLayers doesn't change priority of the background tab.
+      originalTab.linkedBrowser.preserveLayers(true);
+      originalTab.linkedBrowser.renderLayers = true;
+      await new Promise(resolve =>
+        // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+        setTimeout(resolve, WAIT_FOR_CHANGE_TIME_MS)
+      );
+      Assert.equal(
+        gTabPriorityWatcher.currentPriority(origtabID),
+        PROCESS_PRIORITY_BACKGROUND,
+        "Tab didn't get prioritized only due to renderLayers"
+      );
+
       // Test when priorityHint is true, the original tab priority
       // becomes PROCESS_PRIORITY_FOREGROUND.
       originalTab.linkedBrowser.frameLoader.remoteTab.priorityHint = true;
@@ -364,6 +377,9 @@ add_task(async function test_normal_background_tab() {
         PROCESS_PRIORITY_FOREGROUND,
         "Setting priorityHint to false should maintain the new tab priority as foreground"
       );
+
+      originalTab.linkedBrowser.preserveLayers(false);
+      originalTab.linkedBrowser.renderLayers = false;
     }
   );
 });

diff --git a/hal/android/AndroidProcessPriority.cpp b/hal/android/AndroidProcessPriority.cpp
@@ -10,6 +10,8 @@
 #include "mozilla/java/GeckoProcessTypeWrappers.h"
 #include "mozilla/java/ServiceAllocatorWrappers.h"
 
+using namespace mozilla::hal;
+
 /**
  * Bucket the Gecko HAL process priority level into one of the three Android
  * priority levels.

diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoViewTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoViewTest.kt
@@ -95,12 +95,15 @@ class GeckoViewTest : BaseSessionTest() {
         val lowPids = low.map { sessionRule.getSessionPid(it) }.toSet()
 
         UiThreadUtils.waitForCondition({
-            getContentProcessesPriority(highPids).count { it > 100 } == 0
-                    && getContentProcessesPriority(lowPids).count { it < 300 } == 0
+            val shouldBeHighPri = getContentProcessesOomScore(highPids)
+            val shouldBeLowPri = getContentProcessesOomScore(lowPids)
+            // Note that higher oom score means less priority
+            shouldBeHighPri.count { it > 100 } == 0
+                    && shouldBeLowPri.count { it < 300 } == 0
         }, env.defaultTimeoutMillis)
     }
 
-    fun getContentProcessesPriority(pids: Collection<Int>) : List<Int> {
+    fun getContentProcessesOomScore(pids: Collection<Int>) : List<Int> {
         return pids.map { pid ->
             File("/proc/$pid/oom_score").readText(Charsets.UTF_8).trim().toInt()
         }
@@ -162,29 +165,6 @@ class GeckoViewTest : BaseSessionTest() {
         }
     }
 
-    @Test
-    @NullDelegate(Autofill.Delegate::class)
-    fun extensionCurrentTabRaisesPriority() {
-        // Bug 1767346
-        assumeThat(false, equalTo(true))
-
-        val otherSession = setupPriorityTest()
-
-        // Setting priorityHint to PRIORITY_HIGH raises priority
-        mainSession.setPriorityHint(GeckoSession.PRIORITY_HIGH)
-
-        waitUntilContentProcessPriority(
-            high = listOf(mainSession, otherSession), low = listOf()
-        )
-
-        // Setting the priorityHint to default should lower priority
-        mainSession.setPriorityHint(GeckoSession.PRIORITY_DEFAULT)
-
-        waitUntilContentProcessPriority(
-            high = listOf(mainSession), low = listOf(otherSession)
-        )
-    }
-
     @Test
     @NullDelegate(Autofill.Delegate::class)
     fun processPriorityTest() {
@@ -221,8 +201,8 @@ class GeckoViewTest : BaseSessionTest() {
     @Test
     @NullDelegate(Autofill.Delegate::class)
     fun setPriorityHint() {
-        // Bug 1767346
-        assumeThat(false, equalTo(true))
+        // Bug 1768102 - Doesn't seem to work on Fission
+        assumeThat(env.isFission || env.isIsolatedProcess, equalTo(false))
 
         val otherSession = setupPriorityTest()