cc: If SetDeferCommits(true) happens inside the main frame, abort it.

cc/trees/layer_tree_host_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/trees/layer_tree_host.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 2676 matching lines @@
   FakeContentLayerClient client_;
   scoped_refptr<Layer> layer_;
   int num_commit_complete_;
   int num_draw_layers_;
 };
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestContinuousInvalidate);
 
 class LayerTreeHostTestDeferCommits : public LayerTreeHostTest {
  public:
-  LayerTreeHostTestDeferCommits()
-      : num_will_begin_impl_frame_(0), num_send_begin_main_frame_(0) {}
+  LayerTreeHostTestDeferCommits() = default;
 
-  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+  void BeginTest() override {
+    // Start with commits deferred.
+    PostSetDeferCommitsToMainThread(true);
+    PostSetNeedsCommitToMainThread();
+  }
 
   void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
                                   const BeginFrameArgs& args) override {
+    // Impl frames happen while commits are deferred.
     num_will_begin_impl_frame_++;
     switch (num_will_begin_impl_frame_) {
       case 1:
-        break;
       case 2:
       case 3:
       case 4:
         // Post a number of frames to increase the chance that, if there exist
         // bugs, an unexpected BeginMainFrame will be issued.
         PostSetNeedsCommitToMainThread();
         PostSetNeedsRedrawToMainThread();
         break;
       case 5:
-        PostSetDeferCommitsToMainThread(false);
+        MainThreadTaskRunner()->PostTask(
+            FROM_HERE,
+            // Unretained because the test should not end before allowing
+            // commits via this running.
+            base::Bind(&LayerTreeHostTestDeferCommits::AllowCommits,
+                       base::Unretained(this)));
         break;
       default:
         // Sometimes |num_will_begin_impl_frame_| will be greater than 5 if the
         // main thread is slow to respond.
         break;
     }
   }
 
   void WillBeginMainFrame() override {
+    EXPECT_TRUE(allow_commits_);
     num_send_begin_main_frame_++;
-    switch (num_send_begin_main_frame_) {
-      case 1:
-        layer_tree_host()->SetDeferCommits(true);
-        break;
-      case 2:
-        EndTest();
-        break;
-      default:
-        NOTREACHED();
-        break;
-    }
+    EndTest();
+  }
+
+  void AllowCommits() {
+    allow_commits_ = true;
+    layer_tree_host()->SetDeferCommits(false);
   }
 
   void AfterTest() override {
     EXPECT_GE(num_will_begin_impl_frame_, 5);
-    EXPECT_EQ(2, num_send_begin_main_frame_);
+    EXPECT_EQ(1, num_send_begin_main_frame_);
   }
 
  private:
-  int num_will_begin_impl_frame_;
-  int num_send_begin_main_frame_;
+  bool allow_commits_ = false;
+  int num_will_begin_impl_frame_ = 0;
+  int num_send_begin_main_frame_ = 0;
 };
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestDeferCommits);
 
+// This verifies that we can abort a commit inside the main frame, and
+// we don't leave any weird states around if we never allow the commit

[enne (OOO), 2017/03/26 23:50:06]

Thanks for testing both of these states.  :D

+// to happen.
+class LayerTreeHostTestDeferCommitsInsideBeginMainFrame
+    : public LayerTreeHostTest {
+ public:
+  LayerTreeHostTestDeferCommitsInsideBeginMainFrame() = default;
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void WillBeginMainFrame() override {
+    ++begin_main_frame_count_;
+    if (allow_commits_)
+      return;
+
+    // This should prevent the commit from happening.
+    layer_tree_host()->SetDeferCommits(true);
+    // Wait to see if the commit happens. It's possible the deferred

[enne (OOO), 2017/03/26 23:50:06]

I looked at TestHooks and LayerTreeTest, but it doesn't seem like there's
anything there that would happen synchronously that you could assert
allow_commits_ inside of, like UpdateLayers or NotifyReadyToCommit.  I think
this is probably good enough, but it seems like it could be more robust if we
added more hooks for an event that you knew would happen synchronously if the
commit wasn't deferred.  That would also let you not have to wait, as you could
just post an event to EndTest to run immediately.

+    // commit happens when it shouldn't but takes long enough that
+    // this passes. But it won't fail when it shouldn't.
+    MainThreadTaskRunner()->PostDelayedTask(
+        FROM_HERE,
+        // Unretained because the test doesn't end before this runs.
+        base::Bind(&LayerTreeTest::EndTest, base::Unretained(this)),
+        base::TimeDelta::FromMilliseconds(100));
+  }
+
+  void DidCommit() override { ++commit_count_; }
+
+  void AfterTest() override {
+    EXPECT_EQ(0, commit_count_);
+    EXPECT_EQ(1, begin_main_frame_count_);
+  }
+
+ private:
+  bool allow_commits_ = false;
+  int commit_count_ = 0;
+  int begin_main_frame_count_ = 0;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(
+    LayerTreeHostTestDeferCommitsInsideBeginMainFrame);
+
+// This verifies that we can abort a commit inside the main frame, and
+// we will finish the commit once it is allowed.
+class LayerTreeHostTestDeferCommitsInsideBeginMainFrameWithCommitAfter
+    : public LayerTreeHostTest {
+ public:
+  LayerTreeHostTestDeferCommitsInsideBeginMainFrameWithCommitAfter() = default;
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void WillBeginMainFrame() override {
+    ++begin_main_frame_count_;
+    if (allow_commits_)
+      return;
+
+    // This should prevent the commit from happening.
+    layer_tree_host()->SetDeferCommits(true);
+    // Wait to see if the commit happens. It's possible the deferred
+    // commit happens when it shouldn't but takes long enough that
+    // this passes. But it won't fail when it shouldn't.
+    MainThreadTaskRunner()->PostDelayedTask(
+        FROM_HERE,
+        // Unretained because the test doesn't end before this runs.
+        base::Bind(
+            &LayerTreeHostTestDeferCommitsInsideBeginMainFrameWithCommitAfter::
+                AllowCommits,
+            base::Unretained(this)),
+        base::TimeDelta::FromMilliseconds(100));
+  }
+
+  void AllowCommits() {
+    // Once we've waited and seen that commit did not happen, we
+    // allow commits and should see this one go through.
+    allow_commits_ = true;
+    layer_tree_host()->SetDeferCommits(false);
+  }
+
+  void DidCommit() override {
+    ++commit_count_;
+    EXPECT_TRUE(allow_commits_);
+    EndTest();
+  }
+
+  void AfterTest() override {
+    EXPECT_EQ(1, commit_count_);
+    EXPECT_EQ(2, begin_main_frame_count_);
+  }
+
+ private:
+  bool allow_commits_ = false;
+  int commit_count_ = 0;
+  int begin_main_frame_count_ = 0;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(
+    LayerTreeHostTestDeferCommitsInsideBeginMainFrameWithCommitAfter);
+
 class LayerTreeHostTestCompositeImmediatelyStateTransitions
     : public LayerTreeHostTest {
  public:
   enum {
     kInvalid,
     kStartedTest,
     kStartedImplFrame,
     kStartedMainFrame,
     kStartedCommit,
     kCompletedCommit,
@@ skipping 4430 matching lines @@
 
  private:
   bool first_ = true;
   SkBitmap bitmap_;
 };
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestQueueImageDecodeNonLazy);
 
 }  // namespace
 }  // namespace cc