RC: Perform state transitions asynchronously.

Previously, when an event could cause a state transition (e.g. change of
tab visiblity, tab finishes loading...), we called
TabManager::PerformStateTransitions() synchronously. Unfortunately, the
call wasn't always made in a context where performing the state transition
was safe (e.g. can't discard when we are doing a tab switch, because both
the tab switch and the discard touch the TabStripModel).

With this CL, we schedule asynchronous calls to
PerformStateTransitions() when an event could cause a state
transition instead of calling it synchronously, to ensure that state
transitions always occur in a safe context.

Bug: 855053
Change-Id: I5c4f00fb9c4b58393ea7792a05f588614c68f62c
Reviewed-on: https://chromium-review.googlesource.com/1115367
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570517}

chrome/browser/resource_coordinator/tab_manager.cc

@@ -166,7 +166,10 @@ class TabManager::TabManagerSessionRestoreObserver final
 constexpr base::TimeDelta TabManager::kDefaultMinTimeToPurge;
 
 TabManager::TabManager()
-    : browser_tab_strip_tracker_(this, nullptr, nullptr),
+    : state_transitions_callback_(
+          base::BindRepeating(&TabManager::PerformStateTransitions,
+                              base::Unretained(this))),
+      browser_tab_strip_tracker_(this, nullptr, nullptr),
       is_session_restore_loading_tabs_(false),
       restored_tab_count_(0u),
       background_tab_loading_mode_(BackgroundTabLoadingMode::kStaggered),
@@ -609,7 +612,7 @@ void TabManager::OnLoadingStateChange(content::WebContents* web_contents,
     }
 
     // Once a tab is loaded, it might be eligible for freezing.
-    PerformStateTransitions();
+    SchedulePerformStateTransitions(base::TimeDelta());
   }
 }
 
@@ -927,6 +930,16 @@ base::TimeDelta TabManager::GetTimeInBackgroundBeforeProactiveDiscard() const {
   return proactive_freeze_discard_params_.low_occluded_timeout;
 }
 
+void TabManager::SchedulePerformStateTransitions(base::TimeDelta delay) {
+  if (!state_transitions_timer_) {
+    state_transitions_timer_ =
+        std::make_unique<base::OneShotTimer>(GetTickClock());
+  }
+
+  state_transitions_timer_->Start(FROM_HERE, delay,
+                                  state_transitions_callback_);
+}
+
 void TabManager::PerformStateTransitions() {
   if (!base::FeatureList::IsEnabled(features::kProactiveTabFreezeAndDiscard))
     return;
@@ -1010,19 +1023,10 @@ void TabManager::PerformStateTransitions() {
   }
 
   // Schedule the next call to PerformStateTransitions().
-  if (!state_transitions_timer_) {
-    state_transitions_timer_ =
-        std::make_unique<base::OneShotTimer>(GetTickClock());
-  } else {
+  if (next_state_transition_time.is_max() && state_transitions_timer_)
     state_transitions_timer_->Stop();
-  }
-
-  if (!next_state_transition_time.is_max()) {
-    state_transitions_timer_->Start(
-        FROM_HERE, next_state_transition_time - now,
-        base::BindRepeating(&TabManager::PerformStateTransitions,
-                            base::Unretained(this)));
-  }
+  else
+    SchedulePerformStateTransitions(next_state_transition_time - now);
 }
 
 void TabManager::OnLifecycleUnitStateChanged(LifecycleUnit* lifecycle_unit,
@@ -1038,17 +1042,10 @@ void TabManager::OnLifecycleUnitStateChanged(LifecycleUnit* lifecycle_unit,
   } else if (was_discarded && !is_discarded) {
     num_loaded_lifecycle_units_++;
     // Incrementing the number of loaded tabs might change the return value of
-    // GetTimeInBackgroundBeforeProactiveDiscard(). Call
+    // GetTimeInBackgroundBeforeProactiveDiscard(). Schedule a call to
     // PerformStateTransitions() to determine if a tab should be discarded in
     // response to that change.
-    //
-    // Note: PerformStateTransitions() is not called when a tab is frozen or
-    // discarded, because this is unnecessary and could cause recursive calls:
-    //   TabManager::PerformStateTransitions()
-    //     LifecycleUnit::Freeze()
-    //       TabManager::OnLifecycleUnitStateChanged()
-    //         TabManager::PerformStateTransitions()
-    PerformStateTransitions();
+    SchedulePerformStateTransitions(base::TimeDelta());
   }
 
   DCHECK_EQ(num_loaded_lifecycle_units_,
@@ -1058,7 +1055,7 @@ void TabManager::OnLifecycleUnitStateChanged(LifecycleUnit* lifecycle_unit,
 void TabManager::OnLifecycleUnitVisibilityChanged(
     LifecycleUnit* lifecycle_unit,
     content::Visibility visibility) {
-  PerformStateTransitions();
+  SchedulePerformStateTransitions(base::TimeDelta());
 }
 
 void TabManager::OnLifecycleUnitDestroyed(LifecycleUnit* lifecycle_unit) {
@@ -1071,7 +1068,7 @@ void TabManager::OnLifecycleUnitDestroyed(LifecycleUnit* lifecycle_unit) {
   DCHECK_EQ(num_loaded_lifecycle_units_,
             GetNumLoadedLifecycleUnits(lifecycle_units_));
 
-  PerformStateTransitions();
+  SchedulePerformStateTransitions(base::TimeDelta());
 }
 
 void TabManager::OnLifecycleUnitCreated(LifecycleUnit* lifecycle_unit) {
@@ -1085,7 +1082,7 @@ void TabManager::OnLifecycleUnitCreated(LifecycleUnit* lifecycle_unit) {
   DCHECK_EQ(num_loaded_lifecycle_units_,
             GetNumLoadedLifecycleUnits(lifecycle_units_));
 
-  PerformStateTransitions();
+  SchedulePerformStateTransitions(base::TimeDelta());
 }
 
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_manager.h

@@ -405,7 +405,16 @@ class TabManager : public LifecycleUnitObserver,
   // parameters.
   base::TimeDelta GetTimeInBackgroundBeforeProactiveDiscard() const;
 
+  // Schedules a call to PerformStateTransitions() in |delay|. This overrides
+  // any previously scheduled call.
+  void SchedulePerformStateTransitions(base::TimeDelta delay);
+
   // Performs LifecycleUnit state transitions.
+  //
+  // To avoid reentrancy, this is never called synchronously. When a state
+  // transition should happen in response to an event, an asynchronous call to
+  // this is scheduled via SchedulePerformStateTransitions(base::TimeDelta()).
+  // https://crbug.com/855053
   void PerformStateTransitions();
 
   // LifecycleUnitObserver:
@@ -436,6 +445,10 @@ class TabManager : public LifecycleUnitObserver,
   // allow initialization after mock time is setup in unit tests.
   std::unique_ptr<base::OneShotTimer> state_transitions_timer_;
 
+  // Callback for |state_transitions_timer_|. Stored in a member to avoid
+  // repetitive binds.
+  const base::RepeatingClosure state_transitions_callback_;
+
   // A listener to global memory pressure events.
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;