Fix scheduler bug in skipping main frames

cc/scheduler/scheduler_unittest.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/scheduler.h"
 
 #include <stddef.h>
 
 #include <string>
 #include <vector>
@@ skipping 397 matching lines @@
     BeginFrameArgs args =
         CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, now_src());
     fake_external_begin_frame_source_->TestOnBeginFrame(args);
     return args;
   }
 
   FakeExternalBeginFrameSource* fake_external_begin_frame_source() const {
     return fake_external_begin_frame_source_.get();
   }
 
+  void AdvanceAndMissOneFrame();
   void CheckMainFrameSkippedAfterLateCommit(bool expect_send_begin_main_frame);
   void ImplFrameSkippedAfterLateSwapAck(bool swap_ack_before_deadline);
   void ImplFrameNotSkippedAfterLateSwapAck();
   void BeginFramesNotFromClient(BeginFrameSourceType bfs_type);
   void BeginFramesNotFromClient_SwapThrottled(BeginFrameSourceType bfs_type);
   bool BeginMainFrameOnCriticalPath(TreePriority tree_priority,
                                     ScrollHandlerState scroll_handler_state,
                                     base::TimeDelta durations);
 
   std::unique_ptr<base::SimpleTestTickClock> now_src_;
@@ skipping 847 matching lines @@
   client_->Reset();
   scheduler_->DidLoseCompositorFrameSink();
   EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   task_runner().RunPendingTasks();  // Run posted deadline.
   EXPECT_ACTION("ScheduledActionBeginCompositorFrameSinkCreation", client_, 0,
                 3);
   EXPECT_ACTION("RemoveObserver(this)", client_, 1, 3);
   EXPECT_ACTION("SendBeginMainFrameNotExpectedSoon", client_, 2, 3);
 }
 
-void SchedulerTest::CheckMainFrameSkippedAfterLateCommit(
-    bool expect_send_begin_main_frame) {
+void SchedulerTest::AdvanceAndMissOneFrame() {
   // Impl thread hits deadline before commit finishes.
   scheduler_->SetNeedsBeginMainFrame();
   EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   EXPECT_SCOPED(AdvanceFrame());
   EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   scheduler_->NotifyBeginMainFrameStarted(base::TimeTicks());
   scheduler_->NotifyReadyToCommit();
   scheduler_->NotifyReadyToActivate();
   EXPECT_ACTION("AddObserver(this)", client_, 0, 5);
   EXPECT_ACTION("WillBeginImplFrame", client_, 1, 5);
   EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 2, 5);
   EXPECT_ACTION("ScheduledActionCommit", client_, 3, 5);
   EXPECT_ACTION("ScheduledActionActivateSyncTree", client_, 4, 5);
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
+  client_->Reset();
+}
 
-  client_->Reset();
+void SchedulerTest::CheckMainFrameSkippedAfterLateCommit(
+    bool expect_send_begin_main_frame) {
+  AdvanceAndMissOneFrame();
+
   scheduler_->SetNeedsBeginMainFrame();
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   EXPECT_SCOPED(AdvanceFrame());
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
   EXPECT_EQ(expect_send_begin_main_frame,
             scheduler_->MainThreadMissedLastDeadline());
   EXPECT_TRUE(client_->HasAction("WillBeginImplFrame"));
   EXPECT_EQ(expect_send_begin_main_frame,
             client_->HasAction("ScheduledActionSendBeginMainFrame"));
 }
 
+TEST_F(SchedulerTest, MainFrameNotSkippedAfterLateBeginFrame) {
+  // If a begin frame is delivered extremely late (because the browser has
+  // some contention), make sure that the main frame is not skipped even
+  // if it can activate before the deadline.
+  SetUpScheduler(EXTERNAL_BFS);
+  fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);
+
+  AdvanceAndMissOneFrame();
+
+  // Advance frame and create a begin frame.

[sunnyps, 2016/10/12 20:51:46]

Why do we need this extra frame in between?

[enne (OOO), 2016/10/13 17:09:44]

Which extra frame? There's one frame where the main thread misses the deadline
to get into the MissedLastDeadline state.  Then there's this second frame which
is delivered very late.

[sunnyps, 2016/10/13 20:25:57]

Ooh, sorry. Misread that.

+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  BeginFrameArgs args =
+      CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, now_src());
+
+  // Deliver this begin frame super late.
+  now_src_->Advance(BeginFrameArgs::DefaultInterval() * 100);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args);
+
+  scheduler_->SetNeedsBeginMainFrame();

[sunnyps, 2016/10/12 20:51:46]

nit: Can you call SetNeedsBeginMainFrame before sending the begin frame?

[enne (OOO), 2016/10/13 17:09:44]

Done.

+  EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
+  task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
+  EXPECT_EQ(true, scheduler_->MainThreadMissedLastDeadline());
+  EXPECT_TRUE(client_->HasAction("WillBeginImplFrame"));

[sunnyps, 2016/10/12 20:51:46]

nit: Can you use EXPECT_ACTION here?

[enne (OOO), 2016/10/13 17:09:44]

Done.

+  EXPECT_EQ(true, client_->HasAction("ScheduledActionSendBeginMainFrame"));
+}
+
 TEST_F(SchedulerTest, MainFrameSkippedAfterLateCommit) {
   SetUpScheduler(EXTERNAL_BFS);
   fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);
 
   bool expect_send_begin_main_frame = false;
   EXPECT_SCOPED(
       CheckMainFrameSkippedAfterLateCommit(expect_send_begin_main_frame));
 }
 
 // Response times of BeginMainFrame's without the critical path flag set
@@ skipping 1915 matching lines @@
 }
 
 TEST_F(SchedulerTest, BeginMainFrameOnCriticalPath_AHS) {
   EXPECT_FALSE(BeginMainFrameOnCriticalPath(
       SMOOTHNESS_TAKES_PRIORITY,
       ScrollHandlerState::SCROLL_AFFECTS_SCROLL_HANDLER, kSlowDuration));
 }
 
 }  // namespace
 }  // namespace cc