RC: Prevent freezing a tab that is being discarded.

Previously, TabManager::PerformStateTransitions() could be called
repeatedly and needlessly while a tab is being discarded:

- TabManager::PerformStateTransitions()
  Transitions a LifecycleUnit to PENDING_DISCARD.

- TabManager::PerformStateTransitions()
  Calls CanFreeze() to check if the LifecycleUnit can be frozen.
  CanFreeze() returns true even though the LifecycleUnit is already
  being discarded.
  Calls Freeze(), which transitions the LifecycleUnit to
  PENDING_FREEZE.
  Calls CanDiscard() to check if the LifecycleUnit can be discarded.
  CanDiscard() returns true.
  Calls Discard(kProactive) to discard the LifecycleUnit.
  ** Since a LifecycleUnit was discarded, another call to
  PerformStateTransitions() is scheduled immediately. **

- TabManager::PerformStateTransitions()
  Called repeatedly until the LifecycleUnit transitions to DISCARDED.

This CL fixes the issue by returning false from CanFreeze() and
Freeze() for a LifecycleUnit that is being discarded.

Bug: 775644
Change-Id: I4de25dfe7d569f61d0eef988b217203eeae6f006
Reviewed-on: https://chromium-review.googlesource.com/1132343Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574158}

chrome/browser/resource_coordinator/lifecycle_unit_state.mojom

@@ -47,4 +47,6 @@ enum LifecycleUnitStateChangeReason {
   SYSTEM_MEMORY_PRESSURE = 2,
   // Initiated by an extension.
   EXTENSION_INITIATED = 3,
+  // Initiated by a user action (e.g. changing focus, clicking reload).
+  USER_INITIATED = 4,
 };

chrome/browser/resource_coordinator/tab_lifecycle_unit.cc

@@ -113,20 +113,15 @@ bool IsValidStateChange(LifecycleUnitState from,
     }
     case LifecycleUnitState::PENDING_DISCARD: {
       switch (to) {
-        // The WebContents is focused or explicitly reloaded.
-        case LifecycleUnitState::ACTIVE: {
-          return reason == StateChangeReason::BROWSER_INITIATED ||
-                 reason == StateChangeReason::RENDERER_INITIATED;
-        }
         // - Discard(kUrgent|kExternal) is called, or,
         // - The proactive discard can be completed because:
         //   - The freeze timeout expires, or,
         //   - The renderer notifies the browser that the page has been frozen.
         case LifecycleUnitState::DISCARDED:
           return reason == StateChangeReason::BROWSER_INITIATED;
-        // The WebContents IS focused.
+        // The WebContents is focused.
         case LifecycleUnitState::PENDING_FREEZE:
-          return reason == StateChangeReason::BROWSER_INITIATED;
+          return reason == StateChangeReason::USER_INITIATED;
         default:
           return false;
       }
@@ -135,7 +130,7 @@ bool IsValidStateChange(LifecycleUnitState from,
       switch (to) {
         // The WebContents is focused.
         case LifecycleUnitState::ACTIVE:
-          return reason == StateChangeReason::BROWSER_INITIATED;
+          return reason == StateChangeReason::USER_INITIATED;
         default:
           return false;
       }
@@ -266,8 +261,7 @@ void TabLifecycleUnitSource::TabLifecycleUnit::SetFocused(bool focused) {
   switch (GetState()) {
     case LifecycleUnitState::DISCARDED: {
       // Reload the tab.
-      SetState(LifecycleUnitState::ACTIVE,
-               StateChangeReason::BROWSER_INITIATED);
+      SetState(LifecycleUnitState::ACTIVE, StateChangeReason::USER_INITIATED);
       bool loaded = Load();
       DCHECK(loaded);
       break;
@@ -283,7 +277,7 @@ void TabLifecycleUnitSource::TabLifecycleUnit::SetFocused(bool focused) {
       // page is focused, unfreeze it and initiate a transition to ACTIVE.
       freeze_timeout_timer_->Stop();
       SetState(LifecycleUnitState::PENDING_FREEZE,
-               StateChangeReason::BROWSER_INITIATED);
+               StateChangeReason::USER_INITIATED);
       break;
     }
 
@@ -810,8 +804,11 @@ void TabLifecycleUnitSource::TabLifecycleUnit::OnLifecycleUnitStateChanged(
 }
 
 void TabLifecycleUnitSource::TabLifecycleUnit::DidStartLoading() {
-  if (IsDiscardedOrPendingDiscard(GetState()))
-    SetState(LifecycleUnitState::ACTIVE, StateChangeReason::BROWSER_INITIATED);
+  if (IsDiscardedOrPendingDiscard(GetState())) {
+    // This happens when a discarded tab is explicitly reloaded without being
+    // focused first (right-click > Reload).
+    SetState(LifecycleUnitState::ACTIVE, StateChangeReason::USER_INITIATED);
+  }
 }
 
 void TabLifecycleUnitSource::TabLifecycleUnit::OnVisibilityChanged(

chrome/browser/resource_coordinator/tab_manager_browsertest.cc

@@ -1111,6 +1111,58 @@ IN_PROC_BROWSER_TEST_F(TabManagerTestWithTwoTabs,
   expect_state_transition.Wait();
 }
 
+// Verifies the following state transitions for a tab:
+// - Initial state: ACTIVE
+// - Discard(kProactive): ACTIVE->PENDING_DISCARD
+// - Freeze happens in renderer: PENDING_DISCARD->DISCARDED
+// - Focus: DISCARDED->ACTIVE
+IN_PROC_BROWSER_TEST_F(TabManagerTestWithTwoTabs,
+                       TabProactiveDiscardAndFocusToReload) {
+  // Proactively discard the background tab.
+  EXPECT_EQ(LifecycleUnitState::ACTIVE, GetLifecycleUnitAt(1)->GetState());
+  EXPECT_TRUE(GetLifecycleUnitAt(1)->Discard(DiscardReason::kProactive));
+  EXPECT_EQ(LifecycleUnitState::PENDING_DISCARD,
+            GetLifecycleUnitAt(1)->GetState());
+
+  // After the freeze happens in the renderer, the tab is discarded.
+  {
+    ExpectStateTransitionObserver expect_state_transition(
+        GetLifecycleUnitAt(1), LifecycleUnitState::DISCARDED);
+    expect_state_transition.Wait();
+  }
+
+  // When the tab is focused and made visible, it transitions to ACTIVE.
+  tsm()->ActivateTabAt(1, true);
+  GetWebContentsAt(1)->WasShown();
+  EXPECT_EQ(LifecycleUnitState::ACTIVE, GetLifecycleUnitAt(1)->GetState());
+}
+
+// Verifies the following state transitions for a tab:
+// - Initial state: ACTIVE
+// - Discard(kProactive): ACTIVE->PENDING_DISCARD
+// - Freeze(): Disallowed
+// - Freeze happens in renderer: PENDING_DISCARD->DISCARDED
+IN_PROC_BROWSER_TEST_F(TabManagerTestWithTwoTabs,
+                       TabFreezeDisallowedWhenProactivelyDiscarding) {
+  // Proactively discard the background tab.
+  EXPECT_EQ(LifecycleUnitState::ACTIVE, GetLifecycleUnitAt(1)->GetState());
+  EXPECT_TRUE(GetLifecycleUnitAt(1)->Discard(DiscardReason::kProactive));
+  EXPECT_EQ(LifecycleUnitState::PENDING_DISCARD,
+            GetLifecycleUnitAt(1)->GetState());
+
+  // Freezing the tab should be disallowed.
+  DecisionDetails decision_details;
+  EXPECT_FALSE(GetLifecycleUnitAt(1)->CanFreeze(&decision_details));
+  EXPECT_FALSE(GetLifecycleUnitAt(1)->Freeze());
+  EXPECT_EQ(LifecycleUnitState::PENDING_DISCARD,
+            GetLifecycleUnitAt(1)->GetState());
+
+  // The tab should eventually transition to DISCARDED.
+  ExpectStateTransitionObserver expect_state_transition(
+      GetLifecycleUnitAt(1), LifecycleUnitState::DISCARDED);
+  expect_state_transition.Wait();
+}
+
 // Verifies the following state transitions for a tab:
 // - Initial state: ACTIVE
 // - Discard(kUrgent): ACTIVE->DISCARDED