De-flake & re-enable LayerTreeHostCopyRequestTestLayerDestroyed tests

After http://crrev/c/2256225 we had two classes of flakes.
1) The impl_destoryed_ CopyOutputResult would be non empty.
2) The callback_count_ would be incorrect.

The cause of 1) was in the change that introduced the flake we called
SetViewportRectAndScale() after RunUntilIdle(), but
SetViewportRectAndScale() was what prevented the CopyOutputResult
returned to the impl callback from being non-empty. So if we
RunUntilIdle() fast enough we would still prevent the drawing and the
test would pass. So the fix is to immediately call
SetViewportRectAndScale() after destroying the main to ensure its
always fast enough.

The cause of 2) was that RunUntilIdle() runs JUST the currently posted
tasks, so there was a race between RunUntilIdle() being called and
during the deconstruction of the layer PostTasking the callback. The
fix is to only wait for the main callback in frame 3 so we can verify
that it will happen before we destroy the impl layer. And wait for the
impl at the end of the test.

Bug: 1096962
Change-Id: Iad066a2316f2e4df3bdf53379c87a4697d6da8bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2294981Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Stephen Nusko <nuskos@chromium.org>
Auto-Submit: Stephen Nusko <nuskos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790371}

cc/trees/layer_tree_host_unittest_copyrequest.cc

@@ -7,6 +7,8 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "cc/layers/effect_tree_layer_list_iterator.h"
@@ -39,11 +41,6 @@ class LayerTreeHostCopyRequestTest
           ::testing::tuple<TestRendererType, CompositorMode>> {
  public:
   LayerTreeHostCopyRequestTest() : LayerTreeTest(renderer_type()) {}
-  ~LayerTreeHostCopyRequestTest() {
-    // To prevent any tasks posted from previous tests from interfering wait for
-    // the task environment to empty.
-    CCTestSuite::RunUntilIdle();
-  }
 
   TestRendererType renderer_type() const {
     return ::testing::get<0>(GetParam());
@@ -231,9 +228,7 @@ class LayerTreeHostCopyRequestCompletionCausesCommit
     client_.set_bounds(root_->bounds());
   }
 
-  void BeginTest() override {
-    PostSetNeedsCommitToMainThread();
-  }
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
 
   void DidCommit() override {
     int frame = layer_tree_host()->SourceFrameNumber();
@@ -294,10 +289,7 @@ class LayerTreeHostCopyRequestTestLayerDestroyed
     client_.set_bounds(root_->bounds());
   }
 
-  void BeginTest() override {
-    callback_count_ = 0;
-    PostSetNeedsCommitToMainThread();
-  }
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
 
   void DidCommit() override {
     int frame = layer_tree_host()->SourceFrameNumber();
@@ -308,68 +300,69 @@ class LayerTreeHostCopyRequestTestLayerDestroyed
             viz::CopyOutputRequest::ResultFormat::RGBA_BITMAP,
             base::BindOnce(
                 &LayerTreeHostCopyRequestTestLayerDestroyed::CopyOutputCallback,
-                base::Unretained(this))));
+                base::Unretained(this), &main_destroyed_event_)));
         impl_destroyed_->RequestCopyOfOutput(std::make_unique<
                                              viz::CopyOutputRequest>(
             viz::CopyOutputRequest::ResultFormat::RGBA_BITMAP,
             base::BindOnce(
                 &LayerTreeHostCopyRequestTestLayerDestroyed::CopyOutputCallback,
-                base::Unretained(this))));
-        // We expect that the RequestCopyOfOutput won't yet return results until
-        // the main is destroyed. So we RunUntilIdle to ensure no PostTask is
-        // currently queued to return the result.
-        CCTestSuite::RunUntilIdle();
-        EXPECT_EQ(0, callback_count_);
+                base::Unretained(this), &impl_destroyed_event_)));
+        EXPECT_FALSE(main_destroyed_event_.IsSignaled());
+        EXPECT_FALSE(impl_destroyed_event_.IsSignaled());
 
         // Destroy the main thread layer right away.
         main_destroyed_->RemoveFromParent();
         main_destroyed_.reset();
 
-        // Should callback with a NULL bitmap, result will be in a PostTask so
-        // RunUntilIdle().
-        CCTestSuite::RunUntilIdle();
-        EXPECT_EQ(1, callback_count_);
-
         // Prevent drawing so we can't make a copy of the impl_destroyed layer.
         layer_tree_host()->SetViewportRectAndScale(
             gfx::Rect(), 1.f, GetCurrentLocalSurfaceIdAllocation());
         break;
       case 2:
-        // Flush the message loops and make sure the callbacks run.
+        // Flush the message loops.
         layer_tree_host()->SetNeedsCommit();
         break;
       case 3:
-        // No drawing means no readback yet.
-        EXPECT_EQ(1, callback_count_);
+        // There is no timing promise of when we'll get the main callback but if
+        // we wait for it, we should receive it before we destroy the impl and
+        // before the impl callback. The resulting bitmap will be empty because
+        // we destroyed it in the first frame before it got a chance to draw.
+        EXPECT_TRUE(
+            main_destroyed_event_.TimedWait(TestTimeouts::action_timeout()));
+        EXPECT_FALSE(impl_destroyed_event_.IsSignaled());
 
         // Destroy the impl thread layer.
         impl_destroyed_->RemoveFromParent();
         impl_destroyed_.reset();
-
-        // No callback yet because it's on the impl side.
-        EXPECT_EQ(1, callback_count_);
         break;
       case 4:
-        // Flush the message loops and make sure the callbacks run.
+        // Flush the message loops.
         layer_tree_host()->SetNeedsCommit();
         break;
       case 5:
-        // We should get another callback with a NULL bitmap.
-        EXPECT_EQ(2, callback_count_);
+        // There is no timing promise of when we'll get the impl callback but if
+        // we wait for it, we should receive it before the end of the test.
+        // The resulting bitmap will be empty because we called
+        // SetViewportRectAndScale() in the first frame before it got a chance
+        // to draw.
+        EXPECT_TRUE(
+            impl_destroyed_event_.TimedWait(TestTimeouts::action_timeout()));
         EndTest();
         break;
     }
   }
 
-  void CopyOutputCallback(std::unique_ptr<viz::CopyOutputResult> result) {
+  void CopyOutputCallback(base::WaitableEvent* event,
+                          std::unique_ptr<viz::CopyOutputResult> result) {
     EXPECT_TRUE(result->IsEmpty());
-    ++callback_count_;
+    event->Signal();
   }
 
-  int callback_count_;
   FakeContentLayerClient client_;
   scoped_refptr<FakePictureLayer> root_;
+  base::WaitableEvent main_destroyed_event_;
   scoped_refptr<FakePictureLayer> main_destroyed_;
+  base::WaitableEvent impl_destroyed_event_;
   scoped_refptr<FakePictureLayer> impl_destroyed_;
 };
 
@@ -379,8 +372,7 @@ INSTANTIATE_TEST_SUITE_P(
     CombineWithCompositorModes({TestRendererType::kGL,
                                 TestRendererType::kSkiaGL}));
 
-// TODO(crbug/1096962): Investigate flakiness and reenable test once fixed.
-TEST_P(LayerTreeHostCopyRequestTestLayerDestroyed, DISABLED_Test) {
+TEST_P(LayerTreeHostCopyRequestTestLayerDestroyed, Test) {
   RunTest(compositor_mode());
 }
 
@@ -1012,9 +1004,7 @@ class LayerTreeHostCopyRequestTestCountSharedImages
     LayerTreeHostCopyRequestTest::SetupTree();
   }
 
-  void BeginTest() override {
-    PostSetNeedsCommitToMainThread();
-  }
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
 
   virtual void RequestCopy(Layer* layer) = 0;