Send FrameSwapMessageQueue's messages with a separate IPC

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 90afecd8290a9633cee6f2b9de069c68d3f5951a..45051b9ddde31412ef45769e8a669cba21e8a2b9 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -572,6 +572,8 @@ bool RenderWidgetHostImpl::OnMessageReceived(const IPC::Message &msg) {
     IPC_MESSAGE_HANDLER(ViewHostMsg_FocusedNodeTouched, OnFocusedNodeTouched)
     IPC_MESSAGE_HANDLER(DragHostMsg_StartDragging, OnStartDragging)
     IPC_MESSAGE_HANDLER(DragHostMsg_UpdateDragCursor, OnUpdateDragCursor)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_FrameSwapMessages,
+                        OnFrameSwapMessagesReceived)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
@@ -1574,6 +1576,35 @@ void RenderWidgetHostImpl::OnUpdateDragCursor(WebDragOperation current_op) {
     view->UpdateDragCursor(current_op);
 }
 
+void RenderWidgetHostImpl::OnFrameSwapMessagesReceived(
+    uint32_t frame_token,
+    std::vector<IPC::Message> messages) {
+  // We will never receive messages for tokens smaller than |frame_token| so
+  // remove them from the queue.
+  while (queued_tokens_.size() && queued_tokens_.front() < frame_token)

[Fady Samuel, 2017/03/31 16:45:14]

Why would the current frame_token not be at the front of the queue?

[Saman Sami, 2017/04/03 19:22:28]

If we assume the renderer->browser connection is reliable (which is true, unlike
the renderer->gpu->browser connection), then you're right, this should not be
necessary.

+    queued_tokens_.pop_front();
+
+  // If there are no tokens queued, queue |messages| so it can be processed
+  // once a matching token arrives.
+  if (queued_tokens_.empty()) {
+    queued_messages_.push_back(
+        std::make_pair(frame_token, std::move(messages)));
+    return;
+  }
+
+  // If the first element of |queued_tokens_| matches |frame_token|,
+  // process |messages| immediately.
+  if (queued_tokens_.front() == frame_token) {
+    queued_tokens_.pop_front();
+    ProcessSwapMessages(std::move(messages));
+    return;
+  }
+
+  // The first element of |queued_tokens_| is larger than |frame_token|.
+  // Discard |messages| because we will never receive |frame_token| in the
+  // future.
+}
+
 void RenderWidgetHostImpl::RendererExited(base::TerminationStatus status,
                                           int exit_code) {
   if (!renderer_initialized_)
@@ -1858,17 +1889,6 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
 
   SubmitCompositorFrame(local_surface_id, std::move(frame));
 
-  RenderProcessHost* rph = GetProcess();
-  for (std::vector<IPC::Message>::const_iterator i =
-           messages_to_deliver_with_frame.begin();
-       i != messages_to_deliver_with_frame.end();
-       ++i) {
-    rph->OnMessageReceived(*i);
-    if (i->dispatch_error())
-      rph->OnBadMessageReceived(*i);
-  }
-  messages_to_deliver_with_frame.clear();
-
   return true;
 }
 
@@ -2607,6 +2627,8 @@ void RenderWidgetHostImpl::SubmitCompositorFrame(
     return;
   }
 
+  uint32_t frame_token = frame.metadata.frame_token;
+
   last_local_surface_id_ = local_surface_id;
   last_frame_size_ = frame_size;
   last_device_scale_factor_ = device_scale_factor;
@@ -2657,6 +2679,46 @@ void RenderWidgetHostImpl::SubmitCompositorFrame(
       new_content_rendering_timeout_->IsRunning()) {
     new_content_rendering_timeout_->Stop();
   }
+
+  if (frame_token)
+    DidProcessFrame(frame_token);
+}
+
+void RenderWidgetHostImpl::DidProcessFrame(uint32_t frame_token) {
+  // We won't receive frames with tokens smaller than |frame_token| in the
+  // future. Get rid of all messages queued for such frames.
+  while (queued_messages_.size() &&
+         queued_messages_.front().first < frame_token)

[Fady Samuel, 2017/03/31 16:45:14]

This seems like a bug to get into this state? If not, this needs more comments.

[Saman Sami, 2017/04/03 19:22:28]

If a frame was dropped (for example, on gpu crash), this can happen.

+    queued_messages_.pop_front();
+
+  // If there are no queued messages, add |frame_token| to |queued_tokens_| so
+  // that we can process its messages once they arrive.
+  if (queued_messages_.empty()) {
+    queued_tokens_.push_back(frame_token);
+    return;
+  }
+
+  // If the first element of |queued_messages_| belongs to |frame_token|,
+  // process the queued messages.
+  if (queued_messages_.front().first == frame_token) {
+    ProcessSwapMessages(std::move(queued_messages_.front().second));
+    queued_messages_.pop_front();
+    return;
+  }
+
+  // The first element of |queued_messages_| belongs to a larger frame token.
+  // We discard |frame_token| because its messages will never arrive.

[Fady Samuel, 2017/03/31 16:45:14]

A well-behaved renderer should never get into this state right? Maybe a comment
to say that the renderer is misbehaving here? Should we kill the renderer or
report a bad message or something?

[Saman Sami, 2017/04/03 19:22:28]

If we assume the renderer->browser connection is reliable (which is true, unlike
the renderer->gpu->browser connection), then you're right, this should not
happen.

+}
+
+void RenderWidgetHostImpl::ProcessSwapMessages(
+    std::vector<IPC::Message> messages) {
+  RenderProcessHost* rph = GetProcess();
+  for (std::vector<IPC::Message>::const_iterator i = messages.begin();
+       i != messages.end(); ++i) {
+    rph->OnMessageReceived(*i);
+    if (i->dispatch_error())
+      rph->OnBadMessageReceived(*i);
+  }
 }
 
 }  // namespace content