Fix AddVSyncCompleteCallback edge cases

AddVSyncCompleteCallback is meant to be called from inside BeginFrame
and to be called immediately after all observers have been notified.
* Make sure all BeginFrame calls are followed by Complete callbacks, not
  just ones from VSync.
* Rename it to AddBeginFrameCompletionCallback to clarify this.
* Release CHECK that AddVSyncCompleteCallback is only called from with
  BeginFrame

Bug: 885124
Change-Id: Ib2c5c8e7870a4876da2d3d2742ed396e3e87d941
Reviewed-on: https://chromium-review.googlesource.com/c/1315970
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605084}

content/browser/android/synchronous_compositor_sync_call_bridge.cc

@@ -93,8 +93,9 @@ bool SynchronousCompositorSyncCallBridge::WaitAfterVSyncOnUIThread(
     return true;
   }
   window_android_in_vsync_ = window_android;
-  window_android_in_vsync_->AddVSyncCompleteCallback(base::BindOnce(
-      &SynchronousCompositorSyncCallBridge::VSyncCompleteOnUIThread, this));
+  window_android_in_vsync_->AddBeginFrameCompletionCallback(base::BindOnce(
+      &SynchronousCompositorSyncCallBridge::BeginFrameCompleteOnUIThread,
+      this));
   return true;
 }
 
@@ -128,7 +129,7 @@ bool SynchronousCompositorSyncCallBridge::IsRemoteReadyOnUIThread() {
   return remote_state_ == RemoteState::READY;
 }
 
-void SynchronousCompositorSyncCallBridge::VSyncCompleteOnUIThread() {
+void SynchronousCompositorSyncCallBridge::BeginFrameCompleteOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(window_android_in_vsync_);
   window_android_in_vsync_ = nullptr;

content/browser/android/synchronous_compositor_sync_call_bridge.h

@@ -107,8 +107,9 @@ class SynchronousCompositorSyncCallBridge
   friend class base::RefCountedThreadSafe<SynchronousCompositorSyncCallBridge>;
   ~SynchronousCompositorSyncCallBridge();
 
-  // Callback passed to WindowAndroid, runs when the current vsync is completed.
-  void VSyncCompleteOnUIThread();
+  // Callback passed to WindowAndroid, runs when the current begin frame is
+  // completed.
+  void BeginFrameCompleteOnUIThread();
 
   // Process metadata.
   void ProcessFrameMetadataOnUIThread(uint32_t metadata_version,

ui/android/window_android.cc

@@ -5,6 +5,7 @@
 #include "ui/android/window_android.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
@@ -48,14 +49,30 @@ class WindowAndroid::WindowBeginFrameSource : public viz::BeginFrameSource {
 
   void OnVSync(base::TimeTicks frame_time, base::TimeDelta vsync_period);
   void OnPauseChanged(bool paused);
+  void AddBeginFrameCompletionCallback(base::OnceClosure callback);
 
  private:
+  friend class WindowAndroid::ScopedOnBeginFrame;
+
   WindowAndroid* const window_;
   base::ObserverList<viz::BeginFrameObserver>::Unchecked observers_;
   int observer_count_;
   viz::BeginFrameArgs last_begin_frame_args_;
   uint64_t next_sequence_number_;
   bool paused_;
+
+  // Set by ScopedOnBeginFrame.
+  std::vector<base::OnceClosure>* vsync_complete_callbacks_ptr_ = nullptr;
+};
+
+class WindowAndroid::ScopedOnBeginFrame {
+ public:
+  explicit ScopedOnBeginFrame(WindowAndroid::WindowBeginFrameSource* bfs);
+  ~ScopedOnBeginFrame();
+
+ private:
+  WindowAndroid::WindowBeginFrameSource* const begin_frame_source_;
+  std::vector<base::OnceClosure> vsync_complete_callbacks_;
 };
 
 void WindowAndroid::WindowBeginFrameSource::AddObserver(
@@ -83,6 +100,7 @@ void WindowAndroid::WindowBeginFrameSource::AddObserver(
       // BeginFrames.
       last_begin_frame_args_.deadline =
           base::TimeTicks::Now() + last_begin_frame_args_.interval;
+      ScopedOnBeginFrame scope(this);
       obs->OnBeginFrame(last_begin_frame_args_);
     }
   }
@@ -100,6 +118,7 @@ void WindowAndroid::WindowBeginFrameSource::RemoveObserver(
 }
 
 void WindowAndroid::WindowBeginFrameSource::OnGpuNoLongerBusy() {
+  ScopedOnBeginFrame scope(this);
   for (auto& obs : observers_)
     obs.OnBeginFrame(last_begin_frame_args_);
 }
@@ -125,6 +144,28 @@ void WindowAndroid::WindowBeginFrameSource::OnPauseChanged(bool paused) {
     obs.OnBeginFrameSourcePausedChanged(paused_);
 }
 
+void WindowAndroid::WindowBeginFrameSource::AddBeginFrameCompletionCallback(
+    base::OnceClosure callback) {
+  CHECK(vsync_complete_callbacks_ptr_);
+  vsync_complete_callbacks_ptr_->emplace_back(std::move(callback));
+}
+
+WindowAndroid::ScopedOnBeginFrame::ScopedOnBeginFrame(
+    WindowAndroid::WindowBeginFrameSource* bfs)
+    : begin_frame_source_(bfs) {
+  DCHECK(!begin_frame_source_->vsync_complete_callbacks_ptr_);
+  begin_frame_source_->vsync_complete_callbacks_ptr_ =
+      &vsync_complete_callbacks_;
+}
+
+WindowAndroid::ScopedOnBeginFrame::~ScopedOnBeginFrame() {
+  DCHECK_EQ(&vsync_complete_callbacks_,
+            begin_frame_source_->vsync_complete_callbacks_ptr_);
+  begin_frame_source_->vsync_complete_callbacks_ptr_ = nullptr;
+  for (base::OnceClosure& callback : vsync_complete_callbacks_)
+    std::move(callback).Run();
+}
+
 // static
 WindowAndroid* WindowAndroid::FromJavaWindowAndroid(
     const JavaParamRef<jobject>& jwindow_android) {
@@ -180,8 +221,9 @@ void WindowAndroid::AddObserver(WindowAndroidObserver* observer) {
     observer_list_.AddObserver(observer);
 }
 
-void WindowAndroid::AddVSyncCompleteCallback(base::OnceClosure callback) {
-  vsync_complete_callbacks_.emplace_back(std::move(callback));
+void WindowAndroid::AddBeginFrameCompletionCallback(
+    base::OnceClosure callback) {
+  begin_frame_source_->AddBeginFrameCompletionCallback(std::move(callback));
 }
 
 void WindowAndroid::RemoveObserver(WindowAndroidObserver* observer) {
@@ -251,10 +293,6 @@ void WindowAndroid::OnVSync(JNIEnv* env,
 
   begin_frame_source_->OnVSync(frame_time, vsync_period);
 
-  for (base::OnceClosure& callback : vsync_complete_callbacks_)
-    std::move(callback).Run();
-  vsync_complete_callbacks_.clear();
-
   if (needs_begin_frames_)
     RequestVSyncUpdate();
 }

ui/android/window_android.h

@@ -8,7 +8,6 @@
 #include <jni.h>
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
@@ -63,7 +62,9 @@ class UI_ANDROID_EXPORT WindowAndroid : public ViewAndroid {
   viz::BeginFrameSource* GetBeginFrameSource();
 
   // Runs the provided callback as soon as the current vsync was handled.
-  void AddVSyncCompleteCallback(base::OnceClosure callback);
+  // This call is only allowed from inside the OnBeginFrame call from the
+  // BeginFrameSource of this window.
+  void AddBeginFrameCompletionCallback(base::OnceClosure callback);
 
   void SetNeedsAnimate();
   void Animate(base::TimeTicks begin_frame_time);
@@ -96,6 +97,7 @@ class UI_ANDROID_EXPORT WindowAndroid : public ViewAndroid {
 
  private:
   class WindowBeginFrameSource;
+  class ScopedOnBeginFrame;
   friend class DisplayAndroidManager;
   friend class WindowBeginFrameSource;
 
@@ -116,7 +118,6 @@ class UI_ANDROID_EXPORT WindowAndroid : public ViewAndroid {
 
   std::unique_ptr<WindowBeginFrameSource> begin_frame_source_;
   bool needs_begin_frames_;
-  std::vector<base::OnceClosure> vsync_complete_callbacks_;
   float mouse_wheel_scroll_factor_;
   bool vsync_paused_ = false;