Implement FrakeToken Synchronizing for RenderFrameMetadata

Currently RenderFrameMetadata is communicated from the renderer to the browser.
The messages are processed right away, even if Viz may have not yet completed
the processing of the frame associated to the data. This is racy.

This change adds a FrameToken to all of the RenderFrameMetadata messages. The
browser side then uses FrameTokenMessageQueue to synchronize these messages.
They will now be processed once Viz has completed processing the frame and has
notified the browser.

TEST=existing viz_content_browsertests
TBR=piman@chromium.org

Bug: 775103
Change-Id: I52e6d023be5591946c589400374236a2a8e979d6
Reviewed-on: https://chromium-review.googlesource.com/962711Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544072}

content/browser/renderer_host/frame_token_message_queue.cc

@@ -22,49 +22,54 @@ void FrameTokenMessageQueue::DidProcessFrame(uint32_t frame_token) {
 
   last_received_frame_token_ = frame_token;
 
-  while (queued_messages_.size() &&
-         queued_messages_.front().first <= frame_token) {
-    ProcessSwapMessages(std::move(queued_messages_.front().second));
-    queued_messages_.pop();
-  }
+  // Gets the first callback associated with a token after |frame_token| or
+  // callback_map_.end().
+  auto upper_bound = callback_map_.upper_bound(frame_token);
+
+  // std::multimap already sorts on keys, so this will process all enqueued
+  // messages up to the current frame token.
+  for (auto it = callback_map_.begin(); it != upper_bound; ++it)
+    std::move(it->second).Run();
+
+  // Clear all callbacks up to the current frame token.
+  callback_map_.erase(callback_map_.begin(), upper_bound);
 }
 
-void FrameTokenMessageQueue::OnFrameSwapMessagesReceived(
+void FrameTokenMessageQueue::EnqueueOrRunFrameTokenCallback(
     uint32_t frame_token,
-    std::vector<IPC::Message> messages) {
+    base::OnceClosure callback) {
   // Zero token is invalid.
   if (!frame_token) {
     client_->OnInvalidFrameToken(frame_token);
     return;
   }
 
-  // Frame tokens always increase.
-  if (queued_messages_.size() && frame_token <= queued_messages_.back().first) {
-    client_->OnInvalidFrameToken(frame_token);
-    return;
-  }
-
   if (frame_token <= last_received_frame_token_) {
-    ProcessSwapMessages(std::move(messages));
+    std::move(callback).Run();
     return;
   }
+  callback_map_.insert(std::make_pair(frame_token, std::move(callback)));
+}
 
-  queued_messages_.push(std::make_pair(frame_token, std::move(messages)));
+void FrameTokenMessageQueue::OnFrameSwapMessagesReceived(
+    uint32_t frame_token,
+    std::vector<IPC::Message> messages) {
+  EnqueueOrRunFrameTokenCallback(
+      frame_token, base::BindOnce(&FrameTokenMessageQueue::ProcessSwapMessages,
+                                  base::Unretained(this), std::move(messages)));
 }
 
 void FrameTokenMessageQueue::Reset() {
   last_received_frame_token_ = 0;
-  // base::queue does not contain a clear.
-  auto doomed = std::move(queued_messages_);
+  callback_map_.clear();
 }
 
 void FrameTokenMessageQueue::ProcessSwapMessages(
     std::vector<IPC::Message> messages) {
-  for (std::vector<IPC::Message>::const_iterator i = messages.begin();
-       i != messages.end(); ++i) {
-    client_->OnProcessSwapMessage(*i);
-    if (i->dispatch_error())
-      client_->OnMessageDispatchError(*i);
+  for (const IPC::Message& i : messages) {
+    client_->OnProcessSwapMessage(i);
+    if (i.dispatch_error())
+      client_->OnMessageDispatchError(i);
   }
 }
 

content/browser/renderer_host/frame_token_message_queue.h

@@ -5,9 +5,10 @@
 #ifndef CONTENT_BROWSER_RENDERER_HOST_FRAME_TOKEN_MESSAGE_QUEUE_H_
 #define CONTENT_BROWSER_RENDERER_HOST_FRAME_TOKEN_MESSAGE_QUEUE_H_
 
+#include <map>
 #include <vector>
 
-#include "base/containers/queue.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "content/common/content_export.h"
 
@@ -17,17 +18,20 @@ class Message;
 
 namespace content {
 
-// The Renderer sends various IPC::Messages which are not to be processed until
-// after their associated frame has been processed. These messages are provided
-// with a FrameToken to be used for synchronizing.
+// The Renderer sends various messages which are not to be processed until after
+// their associated frame has been processed. These messages are provided with a
+// FrameToken to be used for synchronizing.
 //
 // Viz processes the frames, after which it notifies the Browser of which
 // FrameToken has completed processing.
 //
 // This enqueues all IPC::Messages associated with a FrameToken.
 //
+// Additionally other callbacks can be enqueued with
+// EnqueueOrRunFrameTokenCallback.
+//
 // Upon receipt of DidProcessFrame all IPC::Messages associated with the
-// provided FrameToken are then dispatched.
+// provided FrameToken are then dispatched, and all enqueued callbacks are ran.
 class CONTENT_EXPORT FrameTokenMessageQueue {
  public:
   // Notified of errors in processing messages, as well as of the actual
@@ -52,6 +56,12 @@ class CONTENT_EXPORT FrameTokenMessageQueue {
   // there are any queued messages belonging to it, they will be processed.
   void DidProcessFrame(uint32_t frame_token);
 
+  // Enqueues |callback| to be called upon the arrival of |frame_token| in
+  // DidProcessFrame. However if |frame_token| has already arrived |callback| is
+  // ran immediately.
+  void EnqueueOrRunFrameTokenCallback(uint32_t frame_token,
+                                      base::OnceClosure callback);
+
   // Enqueues the swap messages.
   void OnFrameSwapMessagesReceived(uint32_t frame_token,
                                    std::vector<IPC::Message> messages);
@@ -60,7 +70,7 @@ class CONTENT_EXPORT FrameTokenMessageQueue {
   // consistent incase a new renderer is created.
   void Reset();
 
-  uint32_t size() const { return queued_messages_.size(); }
+  uint32_t size() const { return callback_map_.size(); }
 
  protected:
   // Once both the frame and its swap messages arrive, we call this method to
@@ -75,9 +85,9 @@ class CONTENT_EXPORT FrameTokenMessageQueue {
   // having a token less than or equal to this value will be processed.
   uint32_t last_received_frame_token_ = 0;
 
-  // List of all swap messages that their corresponding frames have not arrived.
+  // Map of all callbacks for which their corresponding frame have not arrived.
   // Sorted by frame token.
-  base::queue<std::pair<uint32_t, std::vector<IPC::Message>>> queued_messages_;
+  std::multimap<uint32_t, base::OnceClosure> callback_map_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameTokenMessageQueue);
 };

content/browser/renderer_host/render_frame_metadata_provider_impl.cc

@@ -4,10 +4,16 @@
 
 #include "content/browser/renderer_host/render_frame_metadata_provider_impl.h"
 
+#include "base/bind.h"
+#include "content/browser/renderer_host/frame_token_message_queue.h"
+
 namespace content {
 
-RenderFrameMetadataProviderImpl::RenderFrameMetadataProviderImpl()
-    : render_frame_metadata_observer_client_binding_(this) {}
+RenderFrameMetadataProviderImpl::RenderFrameMetadataProviderImpl(
+    FrameTokenMessageQueue* frame_token_message_queue)
+    : frame_token_message_queue_(frame_token_message_queue),
+      render_frame_metadata_observer_client_binding_(this),
+      weak_factory_(this) {}
 
 RenderFrameMetadataProviderImpl::~RenderFrameMetadataProviderImpl() = default;
 
@@ -40,16 +46,37 @@ RenderFrameMetadataProviderImpl::LastRenderFrameMetadata() const {
   return last_render_frame_metadata_;
 }
 
-void RenderFrameMetadataProviderImpl::OnRenderFrameMetadataChanged(
-    const cc::RenderFrameMetadata& metadata) {
-  last_render_frame_metadata_ = metadata;
+void RenderFrameMetadataProviderImpl::OnFrameTokenRenderFrameMetadataChanged(
+    cc::RenderFrameMetadata metadata) {
+  last_render_frame_metadata_ = std::move(metadata);
   for (Observer& observer : observers_)
     observer.OnRenderFrameMetadataChanged();
 }
 
-void RenderFrameMetadataProviderImpl::OnFrameSubmissionForTesting() {
+void RenderFrameMetadataProviderImpl::OnFrameTokenFrameSubmissionForTesting() {
   for (Observer& observer : observers_)
     observer.OnRenderFrameSubmission();
 }
 
+void RenderFrameMetadataProviderImpl::OnRenderFrameMetadataChanged(
+    uint32_t frame_token,
+    const cc::RenderFrameMetadata& metadata) {
+  // Both RenderFrameMetadataProviderImpl and FrameTokenMessageQueue are owned
+  // by the same RenderWidgetHostImpl. During shutdown the queue is cleared
+  // without running the callbacks.
+  frame_token_message_queue_->EnqueueOrRunFrameTokenCallback(
+      frame_token,
+      base::BindOnce(&RenderFrameMetadataProviderImpl::
+                         OnFrameTokenRenderFrameMetadataChanged,
+                     weak_factory_.GetWeakPtr(), std::move(metadata)));
+}
+
+void RenderFrameMetadataProviderImpl::OnFrameSubmissionForTesting(
+    uint32_t frame_token) {
+  frame_token_message_queue_->EnqueueOrRunFrameTokenCallback(
+      frame_token, base::BindOnce(&RenderFrameMetadataProviderImpl::
+                                      OnFrameTokenFrameSubmissionForTesting,
+                                  base::Unretained(this)));
+}
+
 }  // namespace content

content/browser/renderer_host/render_frame_metadata_provider_impl.h

@@ -6,12 +6,14 @@
 #define CONTENT_BROWSER_RENDERER_HOST_RENDER_FRAME_METADATA_PROVIDER_IMPL_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "content/common/render_frame_metadata.mojom.h"
 #include "content/public/browser/render_frame_metadata_provider.h"
 #include "mojo/public/cpp/bindings/binding.h"
 
 namespace content {
+class FrameTokenMessageQueue;
 
 // Observes RenderFrameMetadata associated with the submission of a frame for a
 // given RenderWidgetHost. The renderer will notify this when sumitting a
@@ -25,7 +27,8 @@ class RenderFrameMetadataProviderImpl
     : public RenderFrameMetadataProvider,
       public mojom::RenderFrameMetadataObserverClient {
  public:
-  RenderFrameMetadataProviderImpl();
+  explicit RenderFrameMetadataProviderImpl(
+      FrameTokenMessageQueue* frame_token_message_queue);
   ~RenderFrameMetadataProviderImpl() override;
 
   void AddObserver(Observer* observer) override;
@@ -41,19 +44,32 @@ class RenderFrameMetadataProviderImpl
   const cc::RenderFrameMetadata& LastRenderFrameMetadata() const override;
 
  private:
+  // Paired with the mojom::RenderFrameMetadataObserverClient overrides, these
+  // methods are enqueued in |frame_token_message_queue_|. They are invoked when
+  // the browser process receives their associated frame tokens. These then
+  // notify any |observers_|.
+  void OnFrameTokenRenderFrameMetadataChanged(cc::RenderFrameMetadata metadata);
+  void OnFrameTokenFrameSubmissionForTesting();
+
   // mojom::RenderFrameMetadataObserverClient:
   void OnRenderFrameMetadataChanged(
+      uint32_t frame_token,
       const cc::RenderFrameMetadata& metadata) override;
-  void OnFrameSubmissionForTesting() override;
+  void OnFrameSubmissionForTesting(uint32_t frame_token) override;
 
   base::ObserverList<Observer> observers_;
 
   cc::RenderFrameMetadata last_render_frame_metadata_;
 
+  // Not owned.
+  FrameTokenMessageQueue* const frame_token_message_queue_;
+
   mojo::Binding<mojom::RenderFrameMetadataObserverClient>
       render_frame_metadata_observer_client_binding_;
   mojom::RenderFrameMetadataObserverPtr render_frame_metadata_observer_ptr_;
 
+  base::WeakPtrFactory<RenderFrameMetadataProviderImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(RenderFrameMetadataProviderImpl);
 };
 

content/browser/renderer_host/render_widget_host_impl.cc

@@ -371,6 +371,7 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(RenderWidgetHostDelegate* delegate,
       compositor_frame_sink_binding_(this),
       frame_token_message_queue_(
           std::make_unique<FrameTokenMessageQueue>(this)),
+      render_frame_metadata_provider_(frame_token_message_queue_.get()),
       frame_sink_id_(base::checked_cast<uint32_t>(process_->GetID()),
                      base::checked_cast<uint32_t>(routing_id_)),
       weak_factory_(this) {

content/common/render_frame_metadata.mojom

@@ -26,8 +26,8 @@ interface RenderFrameMetadataObserver {
 // RenderFrameMetadataObserver::ReportAllFrameSubmissionsForTesting.
 interface RenderFrameMetadataObserverClient {
   // Notified when RenderFrameMetadata has changed.
-  OnRenderFrameMetadataChanged(RenderFrameMetadata metadata);
+  OnRenderFrameMetadataChanged(uint32 frame_token, RenderFrameMetadata metadata);
 
   // Notified on all frame submissions.
-  OnFrameSubmissionForTesting();
+  OnFrameSubmissionForTesting(uint32 frame_token);
 };

content/renderer/render_frame_metadata_observer_impl.cc

@@ -35,7 +35,8 @@ void RenderFrameMetadataObserverImpl::OnRenderFrameSubmission(
   // these can be reported while testing is enabled.
   bool send_metadata = false;
   if (report_all_frame_submissions_for_testing_enabled_) {
-    render_frame_metadata_observer_client_->OnFrameSubmissionForTesting();
+    render_frame_metadata_observer_client_->OnFrameSubmissionForTesting(
+        frame_token_allocator_->GetOrAllocateFrameToken());
     send_metadata = last_render_frame_metadata_ != metadata;
   } else {
     // Sending |root_scroll_offset| outside of tests would leave the browser
@@ -48,7 +49,7 @@ void RenderFrameMetadataObserverImpl::OnRenderFrameSubmission(
 
   if (send_metadata) {
     render_frame_metadata_observer_client_->OnRenderFrameMetadataChanged(
-        metadata);
+        frame_token_allocator_->GetOrAllocateFrameToken(), metadata);
   }
 
   last_render_frame_metadata_ = metadata;

content/test/BUILD.gn

@@ -1390,6 +1390,7 @@ test("content_unittests") {
     "../browser/renderer_host/compositor_resize_lock_unittest.cc",
     "../browser/renderer_host/cursor_manager_unittest.cc",
     "../browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc",
+    "../browser/renderer_host/frame_token_message_queue_unittest.cc",
     "../browser/renderer_host/input/fling_controller_unittest.cc",
     "../browser/renderer_host/input/gesture_event_queue_unittest.cc",
     "../browser/renderer_host/input/input_router_impl_unittest.cc",