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 5caf7eab7947f091d5262799123294bcac397bac..cd2bf1cc3259e622e6fd66148c7b9d78daa38c05 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -574,6 +574,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()
 
@@ -1576,6 +1578,52 @@ void RenderWidgetHostImpl::OnUpdateDragCursor(WebDragOperation current_op) {
     view->UpdateDragCursor(current_op);
 }
 
+void RenderWidgetHostImpl::OnFrameSwapMessagesReceived(
+    uint32_t frame_token,
+    std::vector<IPC::Message> messages) {
+  // Zero token is invalid.
+  if (!frame_token) {
+    bad_message::ReceivedBadMessage(GetProcess(),
+                                    bad_message::RWH_INVALID_FRAME_TOKEN);
+    return;
+  }
+
+  // Frame tokens always increase.
+  if (queued_messages_.size() && frame_token <= queued_messages_.back().first) {
+    bad_message::ReceivedBadMessage(GetProcess(),
+                                    bad_message::RWH_INVALID_FRAME_TOKEN);
+    return;
+  }
+
+  // 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;
+  }
+
+  // The renderer did not send messages for smaller frame tokens. We kill it
+  // because it's misbehaving.
+  if (queued_tokens_.front() < frame_token) {
+    bad_message::ReceivedBadMessage(GetProcess(),
+                                    bad_message::RWH_INVALID_FRAME_TOKEN);
+    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.

[piman, 2017/04/03 23:48:26]

nit: shouldn't this be consistent with the queued_tokens_.front() < frame_token
case above, and kill the renderer too?

If we're assuming that proper renderer behavior is to sends a token with the
frame if and only if there are messages for it, and that both frames and
messages are FIFO in their respective queue, then it's either one of 2 cases:
- queued_tokens_ is empty, because we haven't received the corresponding frame
yet (messages arrived first)
- or queued_tokens_.front() is frame_token (frame arrived first).

Anything else would be a violation of the protocol, right?

[Saman Sami, 2017/04/03 23:56:01]

I'm making the assumption that frames can be dropped but their messages will
always arrive. Frames won't be dropped right now, but in the future if frame
tokens come from the gpu process then it's possible and I'm trying to make my
implementation as future-proof as possible.

[Saman Sami, 2017/04/03 23:58:09]

Maybe we should actually process the messages if the frame is dropped? Please
let me know what you think.

[piman, 2017/04/04 00:08:11]

Do you have a scenario in mind where frames get dropped? I suspect that yes, it
would make sense to process messages in that case.

[Fady Samuel, 2017/04/04 00:10:10]

If the GPU process crashes, perhaps? Note that this is all subtle so please try
to capture the intent of this code in comments.

[Saman Sami, 2017/04/04 00:13:51]

Yes, GPU can crash before being able to send the token to browser (when frames
actually go to the GPU instead of browser). I'll add more comments.

[Saman Sami, 2017/04/04 00:16:17]

If it makes sense to process the messages of frames that were lost, then I 
guess I can replace queued_tokens_ array with a single integer
last_received_frame_token_ and process every message with token less than or
equal to last_received_frame_token_. That should simplify the code.

[piman, 2017/04/04 00:21:53]

sgtm

+}
+
 void RenderWidgetHostImpl::RendererExited(base::TerminationStatus status,
                                           int exit_code) {
   if (!renderer_initialized_)
@@ -1860,17 +1908,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;
 }
 
@@ -2609,6 +2646,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;
@@ -2659,6 +2698,55 @@ 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) {
+  // Frame tokens always increase.
+  if (queued_tokens_.size() && frame_token <= queued_tokens_.back()) {
+    bad_message::ReceivedBadMessage(GetProcess(),
+                                    bad_message::RWH_INVALID_FRAME_TOKEN);
+    return;
+  }
+
+  // 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)
+    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.
+  // This should not happen. The renderer is misbehaving.
+  bad_message::ReceivedBadMessage(GetProcess(),
+                                  bad_message::RWH_INVALID_FRAME_TOKEN);
+}
+
+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