Deliver IPC messages together with SwapCompositorFrame

content/renderer/render_widget.cc

Index: content/renderer/render_widget.cc
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index dd278d60f679d2ef3d9a7e87294bdc4729a4b99d..17517fca4f2c85212683f6df83c21e954f5d407f 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -37,6 +37,7 @@
 #include "content/renderer/gpu/compositor_output_surface.h"
 #include "content/renderer/gpu/compositor_software_output_device.h"
 #include "content/renderer/gpu/delegated_compositor_output_surface.h"
+#include "content/renderer/gpu/frame_swap_message_queue.h"
 #include "content/renderer/gpu/mailbox_output_surface.h"
 #include "content/renderer/gpu/render_widget_compositor.h"
 #include "content/renderer/ime_event_guard.h"
@@ -154,6 +155,57 @@ ui::TextInputMode ConvertInputMode(const blink::WebString& input_mode) {
 // be spent in input hanlders before input starts getting throttled.
 const int kInputHandlingTimeThrottlingThresholdMicroseconds = 4166;
 
+class QueueMessageSwapPromise : public cc::SwapPromise {
+ public:
+  QueueMessageSwapPromise(
+      scoped_refptr<IPC::SyncMessageFilter> message_sender,
+      scoped_refptr<content::FrameSwapMessageQueue> message_queue,
+      int source_frame_number)
+      : message_sender_(message_sender),
+        message_queue_(message_queue),
+        source_frame_number_(source_frame_number),
+        completed_(false) {
+    DCHECK(message_sender_.get());
+    DCHECK(message_queue_.get());
+  }
+
+  virtual ~QueueMessageSwapPromise() {
+    // The promise should have either been kept or broken before it's deleted.
+    DCHECK(completed_);
+  }
+
+  virtual void DidSwap(cc::CompositorFrameMetadata* metadata) OVERRIDE {
+    DCHECK(!completed_);
+    message_queue_->AdvanceToFrame(source_frame_number_);
+    // The OutputSurface will take care of the Drain+Send.
+    completed_ = true;
+  }
+
+  virtual void DidNotSwap(DidNotSwapReason reason) OVERRIDE {
+    DCHECK(!completed_);
+    // This will also send messages explicitly queued with the WITH_NEXT_SWAP
+    // policy. It makes sense to send them if reason == SWAP_FAILS, but they may
+    // end up being sent prematurely if reason == COMMIT_FAILS.

[mkosiba (inactive), 2014/06/18 14:58:02]

Locally I have a CL which adds policy-specific sub-queues to
FrameSwapMessageQueue which should solve this particular issue, but it's pretty
big so I'd prefer to do that as a follow-up.

Do you have any specific requirements/preferences for the behavior here? Should
messages enqueued with the NEXT_SWAP policy be sent when DidNotSwap(SWAP_FAILS)
is called or should they remain in the queue till we get a successful swap?

[piman, 2014/06/19 23:02:29]

SWAP_FAILS happens when we went all the way to drawing, but there was no damage,
so we skipped. I think this is a valid reason to send immediately once we know
that (we would have sent a frame normally, not sending one is an optimization).

COMMIT_FAILS gathers several cases, that we may want to discriminate for this
purpose:
1- the renderer is hidden
2- we lost the output surface / context
3- we went through the commit but found that nothing had changed

case 3 is like SWAP_FAILS - it's an optimization but for practical purposes, we
would have committed and sent a frame normally, so it's ok to send immediately.
case 2 is definitely valid to delay.
I think case 1 too, but I'd have to double-check the specifics for e.g. resize
or plugin moves.

[mkosiba (inactive), 2014/06/24 18:29:54]

If I'm reading that right it means there are cases where we might not get a 
DidSwap for a long time. I think might be better to send on failures then,
otherwise we're creating an API that works most of the time but can also lead to
deadlocks on rare occasions (at least theoretically, say the Host won't 'unhide'
the view till some IPC arrives but the IPC is waiting for a successful DidSwap).

Does it actually matter if we end up delivering messages 'too early' for cases 1
and 2 ?

[piman, 2014/06/24 19:35:53]

I don't think that would happen. The visibility is managed by the UI side, and
would not wait on the renderer.
Yes, I think so. I would like to use this mechanism to send the
ViewHostMsg_UpdateRect together with the frame. Today it's only sent after we
receive the swap ack from the browser, which means after we've actually drawn a
frame and sent it to the browser (i.e. only when we're visible). Some code on
the browser side relies on that ordering. If we were to send the
ViewHostMsg_UpdateRect too early, this would violate the assumptions.

+    std::vector<IPC::Message> messages;
+    message_queue_->AdvanceToFrame(source_frame_number_);
+    scoped_ptr<content::FrameSwapMessageQueue::SendMessageScope>
+        send_message_scope = message_queue_->AcquireSendMessageScope();
+    message_queue_->DrainMessages(&messages);
+    for (std::vector<IPC::Message>::iterator i = messages.begin();
+         i != messages.end();
+         ++i) {
+      message_sender_->Send(&*i);

[piman, 2014/06/19 23:02:29]

Send is supposed to take ownership of the IPC::Message, it's not ok to pass it
the address of the message in the vector.

I think you want to use the other version of DrainMessages here.

[mkosiba (inactive), 2014/06/24 18:29:54]

argh, right right

+    }
+    completed_ = true;
+  }
+
+ private:
+  scoped_refptr<IPC::SyncMessageFilter> message_sender_;
+  scoped_refptr<content::FrameSwapMessageQueue> message_queue_;
+  int source_frame_number_;
+  bool completed_;

[piman, 2014/06/19 23:02:29]

Is this used only for DCHECKs? Should it be in #ifndef NDEBUG ?

[mkosiba (inactive), 2014/06/24 18:29:54]

Done.

+};
+
 }  // namespace
 
 namespace content {
@@ -397,6 +449,7 @@ RenderWidget::RenderWidget(blink::WebPopupType popup_type,
       body_background_color_(SK_ColorWHITE),
 #endif
       popup_origin_scale_for_emulation_(0.f),
+      frame_swap_message_queue_(new FrameSwapMessageQueue()),
       resizing_mode_selector_(new ResizingModeSelector()),
       context_menu_source_type_(ui::MENU_SOURCE_MOUSE) {
   if (!swapped_out)
@@ -812,7 +865,8 @@ scoped_ptr<cc::OutputSurface> RenderWidget::CreateOutputSurface(bool fallback) {
 #if defined(OS_ANDROID)
   if (SynchronousCompositorFactory* factory =
       SynchronousCompositorFactory::GetInstance()) {
-    return factory->CreateOutputSurface(routing_id());
+    return factory->CreateOutputSurface(routing_id(),
+                                        frame_swap_message_queue_);
   }
 #endif
 
@@ -835,21 +889,22 @@ scoped_ptr<cc::OutputSurface> RenderWidget::CreateOutputSurface(bool fallback) {
   if (command_line.HasSwitch(switches::kEnableDelegatedRenderer)) {
     DCHECK(is_threaded_compositing_enabled_);
     return scoped_ptr<cc::OutputSurface>(
-        new DelegatedCompositorOutputSurface(
-            routing_id(),
-            output_surface_id,
-            context_provider));
+        new DelegatedCompositorOutputSurface(routing_id(),
+                                             output_surface_id,
+                                             context_provider,
+                                             frame_swap_message_queue_));
   }
   if (!context_provider.get()) {
     scoped_ptr<cc::SoftwareOutputDevice> software_device(
         new CompositorSoftwareOutputDevice());
 
-    return scoped_ptr<cc::OutputSurface>(new CompositorOutputSurface(
-        routing_id(),
-        output_surface_id,
-        NULL,
-        software_device.Pass(),
-        true));
+    return scoped_ptr<cc::OutputSurface>(
+        new CompositorOutputSurface(routing_id(),
+                                    output_surface_id,
+                                    NULL,
+                                    software_device.Pass(),
+                                    frame_swap_message_queue_,
+                                    true));
   }
 
   if (command_line.HasSwitch(cc::switches::kCompositeToMailbox)) {
@@ -864,21 +919,21 @@ scoped_ptr<cc::OutputSurface> RenderWidget::CreateOutputSurface(bool fallback) {
       format = cc::RGB_565;
 #endif
     return scoped_ptr<cc::OutputSurface>(
-        new MailboxOutputSurface(
-            routing_id(),
-            output_surface_id,
-            context_provider,
-            scoped_ptr<cc::SoftwareOutputDevice>(),
-            format));
+        new MailboxOutputSurface(routing_id(),
+                                 output_surface_id,
+                                 context_provider,
+                                 scoped_ptr<cc::SoftwareOutputDevice>(),
+                                 frame_swap_message_queue_,
+                                 format));
   }
   bool use_swap_compositor_frame_message = false;
   return scoped_ptr<cc::OutputSurface>(
-      new CompositorOutputSurface(
-          routing_id(),
-          output_surface_id,
-          context_provider,
-          scoped_ptr<cc::SoftwareOutputDevice>(),
-          use_swap_compositor_frame_message));
+      new CompositorOutputSurface(routing_id(),
+                                  output_surface_id,
+                                  context_provider,
+                                  scoped_ptr<cc::SoftwareOutputDevice>(),
+                                  frame_swap_message_queue_,
+                                  use_swap_compositor_frame_message));
 }
 
 void RenderWidget::OnSwapBuffersAborted() {
@@ -1218,6 +1273,45 @@ void RenderWidget::DidCommitCompositorFrame() {
 #endif  // defined(VIDEO_HOLE)
 }
 
+namespace {}  //  namespace
+
+void RenderWidget::QueueMessage(IPC::Message* msg,
+                                MessageDeliveryPolicy policy) {
+  if (!compositor_) {
+    Send(msg);
+    return;
+  }
+
+  if (policy == MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP) {
+    frame_swap_message_queue_->QueueMessage(msg);
+    return;
+  }
+
+  if (policy == MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE &&
+      // No need for lock: this gets changed only on this thread.
+      !compositor_->commitRequested() &&
+      // No need for lock: Messages are only enqueued from this thread, if we
+      // don't have any now, no other thread will add any.
+      !frame_swap_message_queue_->Empty()) {

[mkosiba (inactive), 2014/06/18 14:58:03]

so this should handle FIFO ordering correctly now. We only short-circuit if
there are no messages 'in transit' in the queue.

This runs a small chance of unnecessarily adding a message to the queue if the
queue is completely drained between now and line 1304 but I don't think that
matters (should happen infrequently).

[piman, 2014/06/19 23:02:29]

Did you mean && frame_swap_message_queue_->Empty() (no !) ?

[piman, 2014/06/19 23:02:29]

For that case, you may want to force a commit if
!compositor_->commitRequested(). That way you know the SwapPromise will make
progress (worst case with a COMMIT_FAILS) and you'll eventually send the
message.

[mkosiba (inactive), 2014/06/24 18:29:54]

yes, right, I had this method called HasMessages before and forgot to remove the
negation

[mkosiba (inactive), 2014/06/24 18:29:54]

Done.

+    Send(msg);
+    return;
+  }
+
+  DCHECK(policy == MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE ||
+         policy == MESSAGE_DELIVERY_POLICY_WITH_NEXT_COMMIT);
+  const int source_frame_number = compositor_->GetSourceFrameNumber();
+  bool first_message_for_frame = false;
+  frame_swap_message_queue_->QueueMessage(
+      source_frame_number, msg, &first_message_for_frame);
+  if (first_message_for_frame) {
+    scoped_ptr<cc::SwapPromise> promise(new QueueMessageSwapPromise(
+        RenderThreadImpl::current()->sync_message_filter(),
+        frame_swap_message_queue_,
+        source_frame_number));

[mkosiba (inactive), 2014/06/18 14:58:03]

This way uses less memory since we'll use at most one SwapPromise per
source_frame_number rather than one SwapPromise per message.

+    compositor_->QueueSwapPromise(promise.Pass());
+  }
+}
+
 void RenderWidget::didCommitAndDrawCompositorFrame() {
   // NOTE: Tests may break if this event is renamed or moved. See
   // tab_capture_performancetest.cc.