In ProxyMain::BeginMainFrame, check visibility before setting stage states.

Otherwise if there is a commit request and we early out due to being invisible
before commiting, and we set the max+requested_pipeline_stage_ to
NO_PIPELINE_STAGE, then that commit is lost. For flash this results in a commit
never receiving callback, and the next commit can never be requested, thus
rendering is stalled.

BUG=890008
TEST=cc_unittests
R=piman@chromium,enne@chromium.org,danakj@chromium.org

Change-Id: I240be7bb5496f28c0217ee7ef86533509bafcd16
Reviewed-on: https://chromium-review.googlesource.com/c/1343048
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612872}

cc/test/layer_tree_test.cc

@@ -235,6 +235,11 @@ class LayerTreeHostImplForTesting : public LayerTreeHostImpl {
     test_hooks_->DidFinishImplFrameOnThread(this);
   }
 
+  void WillSendBeginMainFrame() override {
+    LayerTreeHostImpl::WillSendBeginMainFrame();
+    test_hooks_->WillSendBeginMainFrameOnThread(this);
+  }
+
   void DidSendBeginMainFrame() override {
     LayerTreeHostImpl::DidSendBeginMainFrame();
     test_hooks_->DidSendBeginMainFrameOnThread(this);

cc/test/test_hooks.h

@@ -35,6 +35,7 @@ class TestHooks : public AnimationDelegate {
   virtual void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
                                           const viz::BeginFrameArgs& args) {}
   virtual void DidFinishImplFrameOnThread(LayerTreeHostImpl* host_impl) {}
+  virtual void WillSendBeginMainFrameOnThread(LayerTreeHostImpl* host_impl) {}
   virtual void DidSendBeginMainFrameOnThread(LayerTreeHostImpl* host_impl) {}
   virtual void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* host_impl,
                                              CommitEarlyOutReason reason) {}

cc/trees/layer_tree_host_impl.h

@@ -306,6 +306,7 @@ class CC_EXPORT LayerTreeHostImpl
     return viewport_damage_rect_;
   }
 
+  virtual void WillSendBeginMainFrame() {}
   virtual void DidSendBeginMainFrame() {}
   virtual void BeginMainFrameAborted(
       CommitEarlyOutReason reason,

cc/trees/layer_tree_host_unittest_proxy.cc

@@ -505,4 +505,41 @@ class LayerTreeHostProxyTestNeedsCommitFromImpl
 
 SINGLE_THREAD_TEST_F(LayerTreeHostProxyTestNeedsCommitFromImpl);
 
+// Test that a commit is correctly delayed but is not lost when turning
+// invisible, and after turning visible, the commit is executed.
+// This is a regression test for https://crbug.com/890008
+class LayerTreeHostProxyTestDelayedCommitDueToVisibility
+    : public LayerTreeHostProxyTest {
+ protected:
+  LayerTreeHostProxyTestDelayedCommitDueToVisibility() = default;
+  ~LayerTreeHostProxyTestDelayedCommitDueToVisibility() override = default;
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void WillSendBeginMainFrameOnThread(LayerTreeHostImpl*) override {
+    if (!set_invisible_once_) {
+      set_invisible_once_ = true;
+      PostSetVisibleToMainThread(false);
+    }
+  }
+
+  void BeginMainFrameAbortedOnThread(LayerTreeHostImpl*,
+                                     CommitEarlyOutReason reason) override {
+    EXPECT_EQ(CommitEarlyOutReason::ABORTED_NOT_VISIBLE, reason);
+    PostSetVisibleToMainThread(true);
+  }
+
+  void DidCommit() override { EndTest(); }
+
+  void AfterTest() override {}
+
+ private:
+  bool set_invisible_once_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(LayerTreeHostProxyTestDelayedCommitDueToVisibility);
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(
+    LayerTreeHostProxyTestDelayedCommitDueToVisibility);
+
 }  // namespace cc

cc/trees/proxy_impl.cc

@@ -533,6 +533,7 @@ void ProxyImpl::ScheduledActionSendBeginMainFrame(
       host_impl_->EvictedUIResourcesExist();
   begin_main_frame_state->completed_image_decode_requests =
       host_impl_->TakeCompletedImageDecodeRequests();
+  host_impl_->WillSendBeginMainFrame();
   MainThreadTaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(&ProxyMain::BeginMainFrame, proxy_main_weak_ptr_,

cc/trees/proxy_main.cc

@@ -143,6 +143,21 @@ void ProxyMain::BeginMainFrame(
   layer_tree_host_->ImageDecodesFinished(
       std::move(begin_main_frame_state->completed_image_decode_requests));
 
+  // Visibility check needs to happen before setting
+  // max_requested_pipeline_stage_. Otherwise a requested commit could get lost
+  // after tab becomes visible again.
+  if (!layer_tree_host_->IsVisible()) {
+    TRACE_EVENT_INSTANT0("cc", "EarlyOut_NotVisible", TRACE_EVENT_SCOPE_THREAD);
+    std::vector<std::unique_ptr<SwapPromise>> empty_swap_promises;
+    ImplThreadTaskRunner()->PostTask(
+        FROM_HERE, base::BindOnce(&ProxyImpl::BeginMainFrameAbortedOnImpl,
+                                  base::Unretained(proxy_impl_.get()),
+                                  CommitEarlyOutReason::ABORTED_NOT_VISIBLE,
+                                  begin_main_frame_start_time,
+                                  base::Passed(&empty_swap_promises)));
+    return;
+  }
+
   final_pipeline_stage_ = max_requested_pipeline_stage_;
   max_requested_pipeline_stage_ = NO_PIPELINE_STAGE;
 
@@ -181,18 +196,6 @@ void ProxyMain::BeginMainFrame(
       std::max(final_pipeline_stage_, deferred_final_pipeline_stage_);
   deferred_final_pipeline_stage_ = NO_PIPELINE_STAGE;
 
-  if (!layer_tree_host_->IsVisible()) {
-    TRACE_EVENT_INSTANT0("cc", "EarlyOut_NotVisible", TRACE_EVENT_SCOPE_THREAD);
-    std::vector<std::unique_ptr<SwapPromise>> empty_swap_promises;
-    ImplThreadTaskRunner()->PostTask(
-        FROM_HERE, base::BindOnce(&ProxyImpl::BeginMainFrameAbortedOnImpl,
-                                  base::Unretained(proxy_impl_.get()),
-                                  CommitEarlyOutReason::ABORTED_NOT_VISIBLE,
-                                  begin_main_frame_start_time,
-                                  base::Passed(&empty_swap_promises)));
-    return;
-  }
-
   current_pipeline_stage_ = ANIMATE_PIPELINE_STAGE;
 
   // Synchronizes scroll offsets and page scale deltas (for pinch zoom) from the

cc/trees/single_thread_proxy.cc

@@ -700,6 +700,7 @@ void SingleThreadProxy::ScheduledActionSendBeginMainFrame(
       << "BeginMainFrame should only be sent inside a BeginImplFrame";
 #endif
 
+  host_impl_->WillSendBeginMainFrame();
   task_runner_provider_->MainThreadTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&SingleThreadProxy::BeginMainFrame,
                                 weak_factory_.GetWeakPtr(), begin_frame_args));