RC: Use creation time as last active time for tabs open in the background.

Previously, "zero" was used as the last active time for tabs open in
the background, which made them immediately eligible for proactive
freeze and discard. With this CL, their creation time is used as the
last active time, which will prevent them from being proactively frozen
or discarded for at least 10 minutes after creation (note: except
when the number of tabs is considered excessive, see
tab_manager_features.cc).

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

chrome/browser/resource_coordinator/lifecycle_unit.h

@@ -88,9 +88,11 @@ class LifecycleUnit {
   // Returns the current visibility of this LifecycleUnit.
   virtual content::Visibility GetVisibility() const = 0;
 
-  // Returns the last time at which the LifecycleUnit was visible, or
-  // base::TimeTicks::Max() if the LifecycleUnit is currently visible.
-  virtual base::TimeTicks GetLastVisibleTime() const = 0;
+  // Returns TimeTicks::Max() if the LifecycleUnit is currently visible, the
+  // last time at which the LifecycleUnit was visible if it's not currently
+  // visible but has been visible in the past, the LifecycleUnit creation time
+  // otherwise.
+  virtual base::TimeTicks GetLastActiveTime() const = 0;
 
   // Returns the loading state associated with a LifecycleUnit.
   virtual LifecycleUnitLoadingState GetLoadingState() const = 0;

chrome/browser/resource_coordinator/lifecycle_unit_base.cc

@@ -10,9 +10,9 @@
 namespace resource_coordinator {
 
 LifecycleUnitBase::LifecycleUnitBase(content::Visibility visibility)
-    : last_visible_time_(visibility == content::Visibility::VISIBLE
+    : last_active_time_(visibility == content::Visibility::VISIBLE
                             ? base::TimeTicks::Max()
-                             : base::TimeTicks()) {}
+                            : NowTicks()) {}
 
 LifecycleUnitBase::~LifecycleUnitBase() = default;
 
@@ -24,8 +24,8 @@ LifecycleUnitState LifecycleUnitBase::GetState() const {
   return state_;
 }
 
-base::TimeTicks LifecycleUnitBase::GetLastVisibleTime() const {
-  return last_visible_time_;
+base::TimeTicks LifecycleUnitBase::GetLastActiveTime() const {
+  return last_active_time_;
 }
 
 void LifecycleUnitBase::AddObserver(LifecycleUnitObserver* observer) {
@@ -58,9 +58,9 @@ void LifecycleUnitBase::OnLifecycleUnitStateChanged(
 void LifecycleUnitBase::OnLifecycleUnitVisibilityChanged(
     content::Visibility visibility) {
   if (visibility == content::Visibility::VISIBLE)
-    last_visible_time_ = base::TimeTicks::Max();
-  else if (last_visible_time_.is_max())
-    last_visible_time_ = NowTicks();
+    last_active_time_ = base::TimeTicks::Max();
+  else if (last_active_time_.is_max())
+    last_active_time_ = NowTicks();
 
   for (auto& observer : observers_)
     observer.OnLifecycleUnitVisibilityChanged(this, visibility);

chrome/browser/resource_coordinator/lifecycle_unit_base.h

@@ -23,7 +23,7 @@ class LifecycleUnitBase : public LifecycleUnit {
 
   // LifecycleUnit:
   int32_t GetID() const override;
-  base::TimeTicks GetLastVisibleTime() const override;
+  base::TimeTicks GetLastActiveTime() const override;
   LifecycleUnitState GetState() const override;
   void AddObserver(LifecycleUnitObserver* observer) override;
   void RemoveObserver(LifecycleUnitObserver* observer) override;
@@ -60,7 +60,12 @@ class LifecycleUnitBase : public LifecycleUnit {
   // Current state of this LifecycleUnit.
   LifecycleUnitState state_ = LifecycleUnitState::ACTIVE;
 
-  base::TimeTicks last_visible_time_;
+  // TODO(fdoray): Use WebContents::GetLastActiveTime() instead of tracking a
+  // separate last active time here. For this to work,
+  // WebContents::GetLastActiveTime() will have to be updated to return the last
+  // time at which the WebContents was active, rather than the last time at
+  // which it was activated.
+  base::TimeTicks last_active_time_;
 
   base::ObserverList<LifecycleUnitObserver> observers_;
 

chrome/browser/resource_coordinator/lifecycle_unit_base_unittest.cc

@@ -31,11 +31,23 @@ class MockLifecycleUnitObserver : public LifecycleUnitObserver {
   DISALLOW_COPY_AND_ASSIGN(MockLifecycleUnitObserver);
 };
 
+class LifecycleUnitBaseTest : public testing::Test {
+ protected:
+  LifecycleUnitBaseTest() = default;
+
+  base::SimpleTestTickClock test_clock_;
+  ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_{&test_clock_};
+  testing::StrictMock<MockLifecycleUnitObserver> observer_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LifecycleUnitBaseTest);
+};
+
 }  // namespace
 
 // Verify that GetID() returns different ids for different LifecycleUnits, but
 // always the same id for the same LifecycleUnit.
-TEST(LifecycleUnitBaseTest, GetID) {
+TEST_F(LifecycleUnitBaseTest, GetID) {
   TestLifecycleUnit a;
   TestLifecycleUnit b;
   TestLifecycleUnit c;
@@ -51,79 +63,86 @@ TEST(LifecycleUnitBaseTest, GetID) {
 
 // Verify that observers are notified when the state changes and when the
 // LifecycleUnit is destroyed.
-TEST(LifecycleUnitBaseTest, SetStateNotifiesObservers) {
-  testing::StrictMock<MockLifecycleUnitObserver> observer;
+TEST_F(LifecycleUnitBaseTest, SetStateNotifiesObservers) {
   TestLifecycleUnit lifecycle_unit;
-  lifecycle_unit.AddObserver(&observer);
+  lifecycle_unit.AddObserver(&observer_);
 
   // Observer is notified when the state changes.
-  EXPECT_CALL(observer, OnLifecycleUnitStateChanged(&lifecycle_unit,
-                                                    lifecycle_unit.GetState()));
+  EXPECT_CALL(observer_, OnLifecycleUnitStateChanged(
+                             &lifecycle_unit, lifecycle_unit.GetState()));
   lifecycle_unit.SetState(LifecycleUnitState::DISCARDED,
                           LifecycleUnitStateChangeReason::BROWSER_INITIATED);
-  testing::Mock::VerifyAndClear(&observer);
+  testing::Mock::VerifyAndClear(&observer_);
 
   // Observer isn't notified when the state stays the same.
   lifecycle_unit.SetState(LifecycleUnitState::DISCARDED,
                           LifecycleUnitStateChangeReason::BROWSER_INITIATED);
 
-  lifecycle_unit.RemoveObserver(&observer);
+  lifecycle_unit.RemoveObserver(&observer_);
 }
 
 // Verify that observers are notified when the LifecycleUnit is destroyed.
-TEST(LifecycleUnitBaseTest, DestroyNotifiesObservers) {
-  testing::StrictMock<MockLifecycleUnitObserver> observer;
+TEST_F(LifecycleUnitBaseTest, DestroyNotifiesObservers) {
   {
     TestLifecycleUnit lifecycle_unit;
-    lifecycle_unit.AddObserver(&observer);
-    EXPECT_CALL(observer, OnLifecycleUnitDestroyed(&lifecycle_unit));
+    lifecycle_unit.AddObserver(&observer_);
+    EXPECT_CALL(observer_, OnLifecycleUnitDestroyed(&lifecycle_unit));
   }
-  testing::Mock::VerifyAndClear(&observer);
+  testing::Mock::VerifyAndClear(&observer_);
+}
+
+// Verify the initial GetLastActiveTime() of a visible LifecycleUnit.
+TEST_F(LifecycleUnitBaseTest, InitialLastActiveTimeForVisibleLifecycleUnit) {
+  TestLifecycleUnit lifecycle_unit(content::Visibility::VISIBLE);
+  EXPECT_EQ(base::TimeTicks::Max(), lifecycle_unit.GetLastActiveTime());
+}
+
+// Verify the initial GetLastActiveTime() of a hidden LifecycleUnit.
+TEST_F(LifecycleUnitBaseTest, InitialLastActiveTimeForHiddenLifecycleUnit) {
+  TestLifecycleUnit lifecycle_unit(content::Visibility::HIDDEN);
+  EXPECT_EQ(NowTicks(), lifecycle_unit.GetLastActiveTime());
 }
 
 // Verify that observers are notified when the visibility of the LifecyleUnit
-// changes. Verify that GetLastVisibleTime() is updated properly.
-TEST(LifecycleUnitBaseTest, VisibilityChangeNotifiesObserversAndUpdatesTime) {
-  base::SimpleTestTickClock test_clock_;
-  ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(&test_clock_);
-  testing::StrictMock<MockLifecycleUnitObserver> observer;
+// changes. Verify that GetLastActiveTime() is updated properly.
+TEST_F(LifecycleUnitBaseTest, VisibilityChangeNotifiesObserversAndUpdatesTime) {
   TestLifecycleUnit lifecycle_unit;
-  lifecycle_unit.AddObserver(&observer);
+  lifecycle_unit.AddObserver(&observer_);
 
   // Observer is notified when the visibility changes.
   test_clock_.Advance(base::TimeDelta::FromMinutes(1));
   base::TimeTicks last_visible_time = NowTicks();
-  EXPECT_CALL(observer, OnLifecycleUnitVisibilityChanged(
+  EXPECT_CALL(observer_, OnLifecycleUnitVisibilityChanged(
                              &lifecycle_unit, content::Visibility::HIDDEN))
       .WillOnce(testing::Invoke(
           [&](LifecycleUnit* lifecycle_unit, content::Visibility visibility) {
-            EXPECT_EQ(last_visible_time, lifecycle_unit->GetLastVisibleTime());
+            EXPECT_EQ(last_visible_time, lifecycle_unit->GetLastActiveTime());
           }));
   lifecycle_unit.OnLifecycleUnitVisibilityChanged(content::Visibility::HIDDEN);
-  testing::Mock::VerifyAndClear(&observer);
+  testing::Mock::VerifyAndClear(&observer_);
 
   test_clock_.Advance(base::TimeDelta::FromMinutes(1));
-  EXPECT_CALL(observer, OnLifecycleUnitVisibilityChanged(
+  EXPECT_CALL(observer_, OnLifecycleUnitVisibilityChanged(
                              &lifecycle_unit, content::Visibility::OCCLUDED))
       .WillOnce(testing::Invoke(
           [&](LifecycleUnit* lifecycle_unit, content::Visibility visibility) {
-            EXPECT_EQ(last_visible_time, lifecycle_unit->GetLastVisibleTime());
+            EXPECT_EQ(last_visible_time, lifecycle_unit->GetLastActiveTime());
           }));
   lifecycle_unit.OnLifecycleUnitVisibilityChanged(
       content::Visibility::OCCLUDED);
-  testing::Mock::VerifyAndClear(&observer);
+  testing::Mock::VerifyAndClear(&observer_);
 
   test_clock_.Advance(base::TimeDelta::FromMinutes(1));
-  EXPECT_CALL(observer, OnLifecycleUnitVisibilityChanged(
+  EXPECT_CALL(observer_, OnLifecycleUnitVisibilityChanged(
                              &lifecycle_unit, content::Visibility::VISIBLE))
       .WillOnce(testing::Invoke(
           [&](LifecycleUnit* lifecycle_unit, content::Visibility visibility) {
-            EXPECT_TRUE(lifecycle_unit->GetLastVisibleTime().is_max());
+            EXPECT_TRUE(lifecycle_unit->GetLastActiveTime().is_max());
           }));
   lifecycle_unit.OnLifecycleUnitVisibilityChanged(content::Visibility::VISIBLE);
-  testing::Mock::VerifyAndClear(&observer);
+  testing::Mock::VerifyAndClear(&observer_);
 
-  lifecycle_unit.RemoveObserver(&observer);
+  lifecycle_unit.RemoveObserver(&observer_);
 }
 
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_manager.cc

@@ -942,7 +942,7 @@ void TabManager::PerformStateTransitions() {
     DecisionDetails freeze_details;
     if (lifecycle_unit->CanFreeze(&freeze_details)) {
       const base::TimeDelta time_not_visible =
-          now - lifecycle_unit->GetLastVisibleTime();
+          now - lifecycle_unit->GetLastActiveTime();
       const base::TimeDelta time_until_freeze =
           proactive_freeze_discard_params_.freeze_timeout - time_not_visible;
 
@@ -965,8 +965,8 @@ void TabManager::PerformStateTransitions() {
     if (lifecycle_unit->CanDiscard(DiscardReason::kProactive,
                                    &discard_details)) {
       if (!oldest_discardable_lifecycle_unit ||
-          lifecycle_unit->GetLastVisibleTime() <
-              oldest_discardable_lifecycle_unit->GetLastVisibleTime()) {
+          lifecycle_unit->GetLastActiveTime() <
+              oldest_discardable_lifecycle_unit->GetLastActiveTime()) {
         oldest_discardable_lifecycle_unit = lifecycle_unit;
         oldest_discardable_lifecycle_unit_decision_details =
             std::move(discard_details);
@@ -986,7 +986,7 @@ void TabManager::PerformStateTransitions() {
   if (proactive_freeze_discard_params_.should_proactively_discard &&
       oldest_discardable_lifecycle_unit) {
     const base::TimeDelta time_not_visible =
-        now - oldest_discardable_lifecycle_unit->GetLastVisibleTime();
+        now - oldest_discardable_lifecycle_unit->GetLastActiveTime();
     const base::TimeDelta time_until_discard =
         GetTimeInBackgroundBeforeProactiveDiscard() - time_not_visible;
 

chrome/browser/resource_coordinator/tab_manager_stats_collector.cc

@@ -333,10 +333,15 @@ void RecordLifecycleStateChangeUkm(
   }
 
   // Set all visibility related fields.
+  //
+  // |time_since_visible| is:
+  // - Zero if the LifecycleUnit is currently visible.
+  // - Time since creation if the LifecycleUnit was never visible.
+  // - Time since visible if the LifecycleUnit was visible in the past.
   auto visibility = lifecycle_unit->GetVisibility();
   base::TimeDelta time_since_visible;  // Zero.
   if (visibility != content::Visibility::VISIBLE)
-    time_since_visible = NowTicks() - lifecycle_unit->GetLastVisibleTime();
+    time_since_visible = NowTicks() - lifecycle_unit->GetLastActiveTime();
   builder.SetTimeSinceVisibilityStateChangeMs(
       time_since_visible.InMilliseconds());
   builder.SetVisibilityState(static_cast<int64_t>(visibility));

chrome/browser/resource_coordinator/test_lifecycle_unit.cc

@@ -16,6 +16,9 @@ TestLifecycleUnit::TestLifecycleUnit(base::TimeTicks last_focused_time,
       process_handle_(process_handle),
       can_discard_(can_discard) {}
 
+TestLifecycleUnit::TestLifecycleUnit(content::Visibility visibility)
+    : LifecycleUnitBase(visibility) {}
+
 TestLifecycleUnit::~TestLifecycleUnit() {
   OnLifecycleUnitDestroyed();
 }

chrome/browser/resource_coordinator/test_lifecycle_unit.h

@@ -18,6 +18,7 @@ class TestLifecycleUnit : public LifecycleUnitBase {
   TestLifecycleUnit(base::TimeTicks last_focused_time = base::TimeTicks(),
                     base::ProcessHandle process_handle = base::ProcessHandle(),
                     bool can_discard = true);
+  explicit TestLifecycleUnit(content::Visibility visibility);
   ~TestLifecycleUnit() override;
 
   void SetLastFocusedTime(base::TimeTicks last_focused_time) {
@@ -45,7 +46,7 @@ class TestLifecycleUnit : public LifecycleUnitBase {
  private:
   base::TimeTicks last_focused_time_;
   base::ProcessHandle process_handle_;
-  bool can_discard_;
+  bool can_discard_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(TestLifecycleUnit);
 };