Make PageAlmostIdle transitions determinate.

This adds a final timeout to PageAlmostIdle transitions so that they always fire at some point.
This notification is intended to mean "initial load has finished", and it makes sense for this
to fire at some point. This simplifies logic in an upcoming CL which factors
observation logic out of the TabManager into a helper component.

BUG=749789

Change-Id: I1db03f991220eebd33251dc5258a2760042eedef
Reviewed-on: https://chromium-review.googlesource.com/929985
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: lpy <lpy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542614}

services/resource_coordinator/observers/page_signal_generator_impl.cc

@@ -24,6 +24,19 @@ namespace resource_coordinator {
 constexpr base::TimeDelta PageSignalGeneratorImpl::kLoadedAndIdlingTimeout =
     base::TimeDelta::FromSeconds(1);
 
+// This is taken as the 95th percentile of tab loading times on the Windows
+// platform (see SessionRestore.ForegroundTabFirstLoaded). This ensures that
+// all tabs eventually transition to loaded, even if they keep the main task
+// queue busy, or continue loading content.
+// static
+constexpr base::TimeDelta PageSignalGeneratorImpl::kWaitingForIdleTimeout =
+    base::TimeDelta::FromMinutes(1);
+
+// Ensure the timeouts make sense relative to each other.
+static_assert(PageSignalGeneratorImpl::kWaitingForIdleTimeout >
+                  PageSignalGeneratorImpl::kLoadedAndIdlingTimeout,
+              "timeouts must be well ordered");
+
 PageSignalGeneratorImpl::PageSignalGeneratorImpl() = default;
 
 PageSignalGeneratorImpl::~PageSignalGeneratorImpl() = default;
@@ -36,9 +49,14 @@ void PageSignalGeneratorImpl::AddReceiver(
 bool PageSignalGeneratorImpl::ShouldObserve(
     const CoordinationUnitBase* coordination_unit) {
   auto cu_type = coordination_unit->id().type;
+  // Always tracked process CUs. This is used for CPU utilization messages.
+  if (cu_type == CoordinationUnitType::kProcess)
+    return true;
+  if (!resource_coordinator::IsPageAlmostIdleSignalEnabled())
+    return false;
+  // Frame and page CUs are only used for PAI.
   return cu_type == CoordinationUnitType::kFrame ||
-         cu_type == CoordinationUnitType::kPage ||
-         cu_type == CoordinationUnitType::kProcess;
+         cu_type == CoordinationUnitType::kPage;
 }
 
 void PageSignalGeneratorImpl::OnCoordinationUnitCreated(
@@ -47,6 +65,9 @@ void PageSignalGeneratorImpl::OnCoordinationUnitCreated(
   if (cu_type != CoordinationUnitType::kPage)
     return;
 
+  if (!resource_coordinator::IsPageAlmostIdleSignalEnabled())
+    return;
+
   // Create page data exists for this Page CU.
   auto* page_cu = static_cast<const PageCoordinationUnitImpl*>(cu);
   DCHECK(!base::ContainsKey(page_data_, page_cu)); // No data should exist yet.
@@ -58,6 +79,10 @@ void PageSignalGeneratorImpl::OnBeforeCoordinationUnitDestroyed(
   auto cu_type = cu->id().type;
   if (cu_type != CoordinationUnitType::kPage)
     return;
+
+  if (!resource_coordinator::IsPageAlmostIdleSignalEnabled())
+    return;
+
   auto* page_cu = static_cast<const PageCoordinationUnitImpl*>(cu);
   size_t count = page_data_.erase(page_cu);
   DCHECK_EQ(1u, count);  // This should always erase exactly one CU.
@@ -67,6 +92,8 @@ void PageSignalGeneratorImpl::OnFramePropertyChanged(
     const FrameCoordinationUnitImpl* frame_cu,
     const mojom::PropertyType property_type,
     int64_t value) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+
   // Only the network idle state of a frame is of interest.
   if (property_type != mojom::PropertyType::kNetworkAlmostIdle)
     return;
@@ -77,6 +104,8 @@ void PageSignalGeneratorImpl::OnPagePropertyChanged(
     const PageCoordinationUnitImpl* page_cu,
     const mojom::PropertyType property_type,
     int64_t value) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+
   // Only the loading state of a page is of interest.
   if (property_type != mojom::PropertyType::kIsLoading)
     return;
@@ -99,14 +128,19 @@ void PageSignalGeneratorImpl::OnProcessPropertyChanged(
                            page_cu->id(),
                            base::TimeDelta::FromMilliseconds(duration));
     }
-  } else if (property_type == mojom::PropertyType::kMainThreadTaskLoadIsLow) {
-    UpdateLoadIdleStateProcess(process_cu);
+  } else {
+    if (resource_coordinator::IsPageAlmostIdleSignalEnabled() &&
+        property_type == mojom::PropertyType::kMainThreadTaskLoadIsLow) {
+      UpdateLoadIdleStateProcess(process_cu);
+    }
   }
 }
 
 void PageSignalGeneratorImpl::OnPageEventReceived(
     const PageCoordinationUnitImpl* page_cu,
     const mojom::Event event) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+
   // Only the navigation committed event is of interest.
   if (event != mojom::Event::kNavigationCommitted)
     return;
@@ -126,6 +160,8 @@ void PageSignalGeneratorImpl::BindToInterface(
 
 void PageSignalGeneratorImpl::UpdateLoadIdleStateFrame(
     const FrameCoordinationUnitImpl* frame_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+
   // Only main frames are relevant in the load idle state.
   if (!frame_cu->IsMainFrame())
     return;
@@ -139,6 +175,8 @@ void PageSignalGeneratorImpl::UpdateLoadIdleStateFrame(
 
 void PageSignalGeneratorImpl::UpdateLoadIdleStatePage(
     const PageCoordinationUnitImpl* page_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+
   auto* page_data = GetPageData(page_cu);
 
   // Once the cycle is complete state transitions are no longer tracked for this
@@ -150,6 +188,15 @@ void PageSignalGeneratorImpl::UpdateLoadIdleStatePage(
   page_data->idling_timer.Stop();
   base::TimeTicks now = ResourceCoordinatorClock::NowTicks();
 
+  // Determine if the overall timeout has fired.
+  if ((page_data->load_idle_state == kLoadedNotIdling ||
+       page_data->load_idle_state == kLoadedAndIdling) &&
+      (now - page_data->loading_stopped) >= kWaitingForIdleTimeout) {
+    TransitionToLoadedAndIdle(page_cu);
+    return;
+  }
+
+  // Otherwise do normal state transitions.
   switch (page_data->load_idle_state) {
     case kLoadingNotStarted: {
       if (!IsLoading(page_cu))
@@ -162,39 +209,36 @@ void PageSignalGeneratorImpl::UpdateLoadIdleStatePage(
       if (IsLoading(page_cu))
         return;
       page_data->load_idle_state = kLoadedNotIdling;
+      page_data->loading_stopped = now;
       // Let the kLoadedNotIdling state transition evaluate, allowing an
       // effective transition directly from kLoading to kLoadedAndIdling.
       FALLTHROUGH;
     }
 
     case kLoadedNotIdling: {
-      if (!IsIdling(page_cu))
-        return;
-      page_data->load_idle_state = kLoadedAndIdling;
-      page_data->idling_started = now;
+      if (IsIdling(page_cu)) {
+        page_data->load_idle_state = kLoadedAndIdling;
+        page_data->idling_started = now;
+      }
       // Break out of the switch statement and set a timer to check for the
       // next state transition.
       break;
     }
 
     case kLoadedAndIdling: {
-      // If the page is not still idling then transition back a state. The timer
-      // has already been canceled above so future calls will only be due to
-      // potential changes in idling state.
+      // If the page is not still idling then transition back a state.
       if (!IsIdling(page_cu)) {
         page_data->load_idle_state = kLoadedNotIdling;
-        return;
-      }
-
-      // Idling has been occurred long enough then make the last state
-      // transition.
-      if (now - page_data->idling_started >= kLoadedAndIdlingTimeout) {
-        page_data->load_idle_state = kLoadedAndIdle;
-        // Notify observers that the page is loaded and idle.
-        DISPATCH_PAGE_SIGNAL(receivers_, NotifyPageAlmostIdle, page_cu->id());
-        return;
+      } else {
+        // Idling has been happening long enough so make the last state
+        // transition.
+        if (now - page_data->idling_started >= kLoadedAndIdlingTimeout) {
+          TransitionToLoadedAndIdle(page_cu);
+          return;
+        }
       }
-
+      // Break out of the switch statement and set a timer to check for the
+      // next state transition.
       break;
     }
 
@@ -203,24 +247,39 @@ void PageSignalGeneratorImpl::UpdateLoadIdleStatePage(
       NOTREACHED();
   }
 
-  // Getting here means a new timer needs to be set.
-  DCHECK_EQ(kLoadedAndIdling, page_data->load_idle_state);
-  base::TimeDelta idling_timeout =
-      (page_data->idling_started + kLoadedAndIdlingTimeout) - now;
+  // Getting here means a new timer needs to be set. Use the nearer of the two
+  // applicable timeouts.
+  base::TimeDelta timeout =
+      (page_data->loading_stopped + kWaitingForIdleTimeout) - now;
+  if (page_data->load_idle_state == kLoadedAndIdling) {
+    timeout = std::min(
+        timeout, (page_data->idling_started + kLoadedAndIdlingTimeout) - now);
+  }
   page_data->idling_timer.Start(
-      FROM_HERE, idling_timeout,
+      FROM_HERE, timeout,
       base::Bind(&PageSignalGeneratorImpl::UpdateLoadIdleStatePage,
                  base::Unretained(this), page_cu));
 }
 
 void PageSignalGeneratorImpl::UpdateLoadIdleStateProcess(
     const ProcessCoordinationUnitImpl* process_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
   for (auto* frame_cu : process_cu->GetFrameCoordinationUnits())
     UpdateLoadIdleStateFrame(frame_cu);
 }
 
+void PageSignalGeneratorImpl::TransitionToLoadedAndIdle(
+    const PageCoordinationUnitImpl* page_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+  auto* page_data = GetPageData(page_cu);
+  page_data->load_idle_state = kLoadedAndIdle;
+  // Notify observers that the page is loaded and idle.
+  DISPATCH_PAGE_SIGNAL(receivers_, NotifyPageAlmostIdle, page_cu->id());
+}
+
 PageSignalGeneratorImpl::PageData* PageSignalGeneratorImpl::GetPageData(
     const PageCoordinationUnitImpl* page_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
   // There are two ways to enter this function:
   // 1. Via On*PropertyChange calls. The backing PageData is guaranteed to
   //    exist in this case as the lifetimes are managed by the CU graph.
@@ -233,6 +292,7 @@ PageSignalGeneratorImpl::PageData* PageSignalGeneratorImpl::GetPageData(
 
 bool PageSignalGeneratorImpl::IsLoading(
     const PageCoordinationUnitImpl* page_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
   int64_t is_loading = 0;
   if (!page_cu->GetProperty(mojom::PropertyType::kIsLoading, &is_loading))
     return false;
@@ -241,6 +301,7 @@ bool PageSignalGeneratorImpl::IsLoading(
 
 bool PageSignalGeneratorImpl::IsIdling(
     const PageCoordinationUnitImpl* page_cu) {
+  DCHECK(resource_coordinator::IsPageAlmostIdleSignalEnabled());
   // Get the Frame CU for the main frame associated with this page.
   const FrameCoordinationUnitImpl* main_frame_cu =
       page_cu->GetMainFrameCoordinationUnit();

services/resource_coordinator/observers/page_signal_generator_impl.h

@@ -33,6 +33,13 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
   // transitions.
   static const base::TimeDelta kLoadedAndIdlingTimeout;
 
+  // The maximum amount of time post-DidStopLoading a page can be waiting for
+  // an idle state to occur before the page is simply considered loaded anyways.
+  // Since PageAlmostIdle is intended as an "initial loading complete" signal,
+  // it needs to eventually terminate. This is strictly greater than the
+  // kLoadedAndIdlingTimeout.
+  static const base::TimeDelta kWaitingForIdleTimeout;
+
   PageSignalGeneratorImpl();
   ~PageSignalGeneratorImpl() override;
 
@@ -61,12 +68,15 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
       const service_manager::BindSourceInfo& source_info);
 
  private:
+  friend class PageSignalGeneratorImplTest;
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest, IsLoading);
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest, IsIdling);
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest,
                            PageDataCorrectlyManaged);
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest,
-                           PageAlmostIdleTransitions);
+                           PageAlmostIdleTransitionsNoTimeout);
+  FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest,
+                           PageAlmostIdleTransitionsWithTimeout);
 
   // The state transitions for the PageAlmostIdle signal. In general a page
   // transitions through these states from top to bottom.
@@ -96,8 +106,12 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
     // to UpdateLoadIdleState. Is reset to kLoadingNotStarted when a non-same
     // document navigation is committed.
     LoadIdleState load_idle_state;
-    // Marks the point in time when the transition to kLoadedAndIdling occurred.
-    // Used for gating the transition to kLoadedAndIdle.
+    // Marks the point in time when the DidStopLoading signal was received,
+    // transitioning to kLoadedAndNotIdling or kLoadedAndIdling. This is used as
+    // the basis for the kWaitingForIdleTimeout.
+    base::TimeTicks loading_stopped;
+    // Marks the point in time when the last transition to kLoadedAndIdling
+    // occurred. Used for gating the transition to kLoadedAndIdle.
     base::TimeTicks idling_started;
     // A one-shot timer used for transitioning between kLoadedAndIdling and
     // kLoadedAndIdle.
@@ -112,6 +126,9 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
   void UpdateLoadIdleStateProcess(
       const ProcessCoordinationUnitImpl* process_cu);
 
+  // Helper function for transitioning to the final state.
+  void TransitionToLoadedAndIdle(const PageCoordinationUnitImpl* page_cu);
+
   // Convenience accessors for state associated with a |page_cu|.
   PageData* GetPageData(const PageCoordinationUnitImpl* page_cu);
   bool IsLoading(const PageCoordinationUnitImpl* page_cu);

services/resource_coordinator/observers/page_signal_generator_impl_unittest.cc

@@ -4,18 +4,38 @@
 
 #include "services/resource_coordinator/observers/page_signal_generator_impl.h"
 
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "services/resource_coordinator/coordination_unit/coordination_unit_test_harness.h"
 #include "services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/mock_coordination_unit_graphs.h"
 #include "services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
+#include "services/resource_coordinator/public/cpp/resource_coordinator_features.h"
 #include "services/resource_coordinator/resource_coordinator_clock.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace resource_coordinator {
 
+// A wrapper for a tick clock. ResourceCoordinatorClock wants TickClock
+// ownership, but the test harness only provides a raw pointer.
+class TickClockWrapper : public base::TickClock {
+ public:
+  explicit TickClockWrapper(base::TickClock* tick_clock)
+      : tick_clock_(tick_clock) {}
+
+  ~TickClockWrapper() override {}
+
+  // base::TickClock implementation:
+  base::TimeTicks NowTicks() override { return tick_clock_->NowTicks(); }
+
+ private:
+  base::TickClock* tick_clock_;
+
+  DISALLOW_COPY_AND_ASSIGN(TickClockWrapper);
+};
+
 class MockPageSignalGeneratorImpl : public PageSignalGeneratorImpl {
  public:
   // Overridden from PageSignalGeneratorImpl.
@@ -34,12 +54,23 @@ class MockPageSignalGeneratorImpl : public PageSignalGeneratorImpl {
 
 class PageSignalGeneratorImplTest : public CoordinationUnitTestHarness {
  protected:
+  void TearDown() override { ResourceCoordinatorClock::ResetClockForTesting(); }
+
   MockPageSignalGeneratorImpl* page_signal_generator() {
     return &page_signal_generator_;
   }
 
+  void EnablePAI() {
+    feature_list_ = std::make_unique<base::test::ScopedFeatureList>();
+    feature_list_->InitAndEnableFeature(features::kPageAlmostIdle);
+    ASSERT_TRUE(resource_coordinator::IsPageAlmostIdleSignalEnabled());
+  }
+
+  void TestPageAlmostIdleTransitions(bool timeout);
+
  private:
   MockPageSignalGeneratorImpl page_signal_generator_;
+  std::unique_ptr<base::test::ScopedFeatureList> feature_list_;
 };
 
 TEST_F(PageSignalGeneratorImplTest,
@@ -61,6 +92,7 @@ TEST_F(PageSignalGeneratorImplTest,
 }
 
 TEST_F(PageSignalGeneratorImplTest, IsLoading) {
+  EnablePAI();
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
   auto* page_cu = cu_graph.page.get();
   auto* psg = page_signal_generator();
@@ -80,6 +112,7 @@ TEST_F(PageSignalGeneratorImplTest, IsLoading) {
 }
 
 TEST_F(PageSignalGeneratorImplTest, IsIdling) {
+  EnablePAI();
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
   auto* frame_cu = cu_graph.frame.get();
   auto* page_cu = cu_graph.page.get();
@@ -114,6 +147,7 @@ TEST_F(PageSignalGeneratorImplTest, IsIdling) {
 }
 
 TEST_F(PageSignalGeneratorImplTest, PageDataCorrectlyManaged) {
+  EnablePAI();
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
   auto* page_cu = cu_graph.page.get();
   auto* psg = page_signal_generator();
@@ -127,12 +161,11 @@ TEST_F(PageSignalGeneratorImplTest, PageDataCorrectlyManaged) {
   EXPECT_EQ(0u, psg->page_data_.count(page_cu));
 }
 
-TEST_F(PageSignalGeneratorImplTest, PageAlmostIdleTransitions) {
+void PageSignalGeneratorImplTest::TestPageAlmostIdleTransitions(bool timeout) {
+  EnablePAI();
   ResourceCoordinatorClock::SetClockForTesting(
-      std::make_unique<base::SimpleTestTickClock>());
-  auto* test_clock = static_cast<base::SimpleTestTickClock*>(
-      ResourceCoordinatorClock::GetClockForTesting());
-  test_clock->Advance(base::TimeDelta::FromSeconds(1));
+      std::make_unique<TickClockWrapper>(task_env().GetMockTickClock()));
+  task_env().FastForwardBy(base::TimeDelta::FromSeconds(1));
 
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
   auto* frame_cu = cu_graph.frame.get();
@@ -185,24 +218,37 @@ TEST_F(PageSignalGeneratorImplTest, PageAlmostIdleTransitions) {
   EXPECT_EQ(LIS::kLoadedAndIdling, page_data->load_idle_state);
   EXPECT_TRUE(page_data->idling_timer.IsRunning());
 
-  // Go back to not idling. We should transition back to kLoadedNotIdling.
+  // Go back to not idling. We should transition back to kLoadedNotIdling, and
+  // a timer should still be running.
   frame_cu->SetNetworkAlmostIdle(false);
   EXPECT_EQ(LIS::kLoadedNotIdling, page_data->load_idle_state);
-  EXPECT_FALSE(page_data->idling_timer.IsRunning());
-
-  // Go back to idling.
-  frame_cu->SetNetworkAlmostIdle(true);
-  EXPECT_EQ(LIS::kLoadedAndIdling, page_data->load_idle_state);
   EXPECT_TRUE(page_data->idling_timer.IsRunning());
 
-  // Let the timer evaluate. The final state transition should occur.
-  // TODO(chrisha): Expose the tick clock from the ScopedTaskEnvironment, and
-  // use that clock in the ResourceCoordinatorClock. Beats having two separate
-  // fake clocks that need to be manually kept in sync.
-  test_clock->Advance(PageSignalGeneratorImpl::kLoadedAndIdlingTimeout);
-  task_env().FastForwardUntilNoTasksRemain();
-  EXPECT_EQ(LIS::kLoadedAndIdle, page_data->load_idle_state);
-  EXPECT_FALSE(page_data->idling_timer.IsRunning());
+  base::TimeTicks start = ResourceCoordinatorClock::NowTicks();
+  if (timeout) {
+    // Let the timeout run down. The final state transition should occur.
+    task_env().FastForwardUntilNoTasksRemain();
+    base::TimeTicks end = ResourceCoordinatorClock::NowTicks();
+    base::TimeDelta elapsed = end - start;
+    EXPECT_LE(PageSignalGeneratorImpl::kLoadedAndIdlingTimeout, elapsed);
+    EXPECT_LE(PageSignalGeneratorImpl::kWaitingForIdleTimeout, elapsed);
+    EXPECT_EQ(LIS::kLoadedAndIdle, page_data->load_idle_state);
+    EXPECT_FALSE(page_data->idling_timer.IsRunning());
+  } else {
+    // Go back to idling.
+    frame_cu->SetNetworkAlmostIdle(true);
+    EXPECT_EQ(LIS::kLoadedAndIdling, page_data->load_idle_state);
+    EXPECT_TRUE(page_data->idling_timer.IsRunning());
+
+    // Let the idle timer evaluate. The final state transition should occur.
+    task_env().FastForwardUntilNoTasksRemain();
+    base::TimeTicks end = ResourceCoordinatorClock::NowTicks();
+    base::TimeDelta elapsed = end - start;
+    EXPECT_LE(PageSignalGeneratorImpl::kLoadedAndIdlingTimeout, elapsed);
+    EXPECT_GT(PageSignalGeneratorImpl::kWaitingForIdleTimeout, elapsed);
+    EXPECT_EQ(LIS::kLoadedAndIdle, page_data->load_idle_state);
+    EXPECT_FALSE(page_data->idling_timer.IsRunning());
+  }
 
   // Firing other signals should not change the state at all.
   proc_cu->SetMainThreadTaskLoadIsLow(false);
@@ -218,4 +264,12 @@ TEST_F(PageSignalGeneratorImplTest, PageAlmostIdleTransitions) {
   EXPECT_FALSE(page_data->idling_timer.IsRunning());
 }
 
+TEST_F(PageSignalGeneratorImplTest, PageAlmostIdleTransitionsNoTimeout) {
+  TestPageAlmostIdleTransitions(false);
+}
+
+TEST_F(PageSignalGeneratorImplTest, PageAlmostIdleTransitionsWithTimeout) {
+  TestPageAlmostIdleTransitions(true);
+}
+
 }  // namespace resource_coordinator