Send FrameSwapMessageQueue's messages with a separate IPC

Send FrameSwapMessageQueue's messages with a separate IPC

We can't put messages inside ViewHostMsg_SwapCompositorFrame once it's
mojoified, so those messages need to be sent separately. In order
to achieve this, I added an integer token to CompositorFrameMetadata
and send the same token with the messages to the browser. The browser
uses the token to match frames and messages and once both the frame and
its messages arrive, the messages are processed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972
Review-Url: https://codereview.chromium.org/2789773003
Cr-Commit-Position: refs/heads/master@{#461962}
Committed: https://chromium.googlesource.com/chromium/src/+/d8ecdc4a0429d984fde83e08a1bb343af2d6851b

[Saman Sami, 2017-03-30 22:37:34]

Description was changed from

==========
Frame token
==========

to

==========
Frame token
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-30 22:40:31]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:41:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 00:08:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 00:08:53]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-31 15:27:18]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-03-31 15:27:39]

Description was changed from

==========
Frame token
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Frame token
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-31 15:27:39]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[commit-bot: I haz the power, 2017-03-31 15:27:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-31 15:29:04]

Please comment on the overall approach. I'm assuming that either pipe can be
busted and some messages can be dropped, hence the added complexity. But I don't
think the logic is too complicated, so I would rather have the robustness than a
simpler code.

[commit-bot: I haz the power, 2017-03-31 15:30:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 15:30:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2017-03-31 15:32:42]

Patchset #4 (id:60001) has been deleted

[Saman Sami, 2017-03-31 15:33:18]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 15:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1584: while
(queued_tokens_.size() && queued_tokens_.front() < frame_token)
Why would the current frame_token not be at the front of the queue?

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2691:
queued_messages_.front().first < frame_token)
This seems like a bug to get into this state? If not, this needs more comments.

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2710: // We discard
|frame_token| because its messages will never arrive.
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?

https://codereview.chromium.org/2789773003/diff/80001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/2789773003/diff/80001/content/common/view_mes...
content/common/view_messages.h:806:
IPC_MESSAGE_ROUTED2(ViewHostMsg_FrameSwapMessages,
I really don't like this name...I don't like "swap" at all, because we don't
really "swap"...it's Submit.

ViewHostMsg_CompositorFrameAlignedMessages?

https://codereview.chromium.org/2789773003/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2789773003/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:53:
last_used_frame_token_(compositor_frame_sink_id << 28) {
I'd rather FrameToken be a struct with two ints like FrameSinkId instead of a
single int with a bunch of bit shifting.. It's easier to read and understand
that way.

[commit-bot: I haz the power, 2017-03-31 17:14:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 17:14:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1584: while
(queued_tokens_.size() && queued_tokens_.front() < frame_token)
On 2017/03/31 16:45:14, Fady Samuel wrote:
> Why would the current frame_token not be at the front of the queue?

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.

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2691:
queued_messages_.front().first < frame_token)
On 2017/03/31 16:45:14, Fady Samuel wrote:
> This seems like a bug to get into this state? If not, this needs more
comments.

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

https://codereview.chromium.org/2789773003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2710: // We discard
|frame_token| because its messages will never arrive.
On 2017/03/31 16:45:14, Fady Samuel wrote:
> 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?

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.

https://codereview.chromium.org/2789773003/diff/80001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/2789773003/diff/80001/content/common/view_mes...
content/common/view_messages.h:806:
IPC_MESSAGE_ROUTED2(ViewHostMsg_FrameSwapMessages,
On 2017/03/31 16:45:14, Fady Samuel wrote:
> I really don't like this name...I don't like "swap" at all, because we don't
> really "swap"...it's Submit.
> 
> ViewHostMsg_CompositorFrameAlignedMessages?

I chose that name because of FrameSwapMessageQueue. "Swap" doesn't make sense if
we take it literally, but we already use it extensively to mean submitting
frames, right?

https://codereview.chromium.org/2789773003/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2789773003/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:53:
last_used_frame_token_(compositor_frame_sink_id << 28) {
On 2017/03/31 16:45:14, Fady Samuel wrote:
> I'd rather FrameToken be a struct with two ints like FrameSinkId instead of a
> single int with a bunch of bit shifting.. It's easier to read and understand
> that way.

What should those two ints be called? Frame token is a general-purpose token
that any two parent / child should be able to use. I'm afraid that we might make
it too browser/renderer-specific by splitting it into two ints because that's
renderer's implementation to some extent.

[Saman Sami, 2017-04-03 19:46:46]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-04-03 19:46:56]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 19:47:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-03 20:08:49]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 20:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-03 20:15:14]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 20:15:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 21:30:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 21:30:50]

Dry run: This issue passed the CQ dry run.

[piman, 2017-04-03 23:48:25]

piman@chromium.org changed reviewers:
+ piman@chromium.org

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

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
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?

https://codereview.chromium.org/2789773003/diff/160001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:53:
last_used_frame_token_(compositor_frame_sink_id << 28) {
Is the <<28 to deal with recreating the RendererCompositorFrameSink? I think we
can deal with this by simply sending the compositor_frame_sink_id with
ViewHostMsg_FrameSwapMessages to make sure we properly scope the frame tokens
with the corresponding RendererCompositorFrameSink. An alternative is to keep
the last_used_frame_token in FrameSwapMessageQueue, so that it survives
RendererCompositorFrameSink recreation.

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

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/03 23:48:26, piman wrote:
> 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?

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.

https://codereview.chromium.org/2789773003/diff/160001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:53:
last_used_frame_token_(compositor_frame_sink_id << 28) {
On 2017/04/03 23:48:26, piman wrote:
> Is the <<28 to deal with recreating the RendererCompositorFrameSink? I think
we
> can deal with this by simply sending the compositor_frame_sink_id with
> ViewHostMsg_FrameSwapMessages to make sure we properly scope the frame tokens
> with the corresponding RendererCompositorFrameSink. An alternative is to keep
> the last_used_frame_token in FrameSwapMessageQueue, so that it survives
> RendererCompositorFrameSink recreation.

Keeping the frame token in FrameSwapMessageQueue is a very good idea!

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

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/03 23:56:01, Saman Sami wrote:
> On 2017/04/03 23:48:26, piman wrote:
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/03 23:58:09, Saman Sami wrote:
> On 2017/04/03 23:56:01, Saman Sami wrote:
> > On 2017/04/03 23:48:26, piman wrote:
> > > 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?
> > 
> > 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.
> 
> Maybe we should actually process the messages if the frame is dropped? Please
> let me know what you think.

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:11]

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/04 00:08:11, piman wrote:
> On 2017/04/03 23:58:09, Saman Sami wrote:
> > On 2017/04/03 23:56:01, Saman Sami wrote:
> > > On 2017/04/03 23:48:26, piman wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > Maybe we should actually process the messages if the frame is dropped?
Please
> > let me know what you think.
> 
> 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.

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]

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/04 00:10:10, Fady Samuel wrote:
> On 2017/04/04 00:08:11, piman wrote:
> > On 2017/04/03 23:58:09, Saman Sami wrote:
> > > On 2017/04/03 23:56:01, Saman Sami wrote:
> > > > On 2017/04/03 23:48:26, piman wrote:
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > Maybe we should actually process the messages if the frame is dropped?
> Please
> > > let me know what you think.
> > 
> > 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.
> 
> If the GPU process crashes, perhaps? Note that this is all subtle so please
try
> to capture the intent of this code in comments.

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]

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
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:30]

On Mon, Apr 3, 2017 at 5:10 PM, <fsamuel@chromium.org> wrote:

>
> https://codereview.chromium.org/2789773003/diff/160001/
> content/browser/renderer_host/render_widget_host_impl.cc
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
>
> https://codereview.chromium.org/2789773003/diff/160001/
> content/browser/renderer_host/render_widget_host_impl.cc#newcode1624
> content/browser/renderer_host/render_widget_host_impl.cc:1624: //
> future.
> On 2017/04/04 00:08:11, piman wrote:
> > On 2017/04/03 23:58:09, Saman Sami wrote:
> > > On 2017/04/03 23:56:01, Saman Sami wrote:
> > > > On 2017/04/03 23:48:26, piman wrote:
> > > > > 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?
> > > >
> > > > 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.
> > >
> > > Maybe we should actually process the messages if the frame is
> dropped? Please
> > > let me know what you think.
> >
> > 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.
>
> If the GPU process crashes, perhaps? Note that this is all subtle so
> please try to capture the intent of this code in comments.
>

Ok, yeah, I think in that scenario we can assume the frame token is legit.
We don't have information on whether the frame actually got received, but
dispatching the message seems the least harmful outcome. I suspect we may
need higher level logic based on what the message dispatcher intended for
the case where the GPU process crashed, but that's a bit orthogonal.


>
> https://codereview.chromium.org/2789773003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1624: // future.
On 2017/04/04 00:16:17, Saman Sami wrote:
> 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.

sgtm

[Saman Sami, 2017-04-04 14:41:44]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 14:42:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 15:24:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 15:24:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2017-04-04 15:44:59]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 15:45:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-04 15:59:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 15:59:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-04 16:24:08]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 16:24:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-04 17:08:07]

Description was changed from

==========
Frame token
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Send FrameSwapMessageQueue's messages with a separate IPC

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972
==========

[Saman Sami, 2017-04-04 17:08:22]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 17:09:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-04 17:15:02]

Description was changed from

==========
Send FrameSwapMessageQueue's messages with a separate IPC

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972
==========

to

==========
Send FrameSwapMessageQueue's messages with a separate IPC

We can't put messages inside ViewHostMsg_SwapCompositorFrame once it's
mojoified, so those messages need to be sent separately. In order
to achieve this, I added an integer token to CompositorFrameMetadata
and send the same token with the messages to the browser. The browser
uses the token to match frames and messages and once both the frame and
its messages arrive, the messages are processed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972
==========

[Saman Sami, 2017-04-04 17:31:41]

samans@chromium.org changed reviewers:
+ isherman@chromium.org

[Saman Sami, 2017-04-04 17:32:19]

samans@chromium.org changed reviewers:
+ tsepez@chromium.org

[Saman Sami, 2017-04-04 17:35:54]

samans@chromium.org changed reviewers:
- tsepez@chromium.org

[Saman Sami, 2017-04-04 17:55:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 17:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-04 17:58:13]

samans@chromium.org changed reviewers:
+ tsepez@chromium.org

[Saman Sami, 2017-04-04 18:02:59]

tsepez@: Please review IPC
piman@: Please review content/
isherman@: Please review metrics

[Tom Sepez, 2017-04-04 18:11:04]

https://codereview.chromium.org/2789773003/diff/300001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_metadata.mojom (right):

https://codereview.chromium.org/2789773003/diff/300001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_metadata.mojom:37: uint32 frame_token;
Does it matter that these are guessable?
It looks like we shoot the renderer if it sends a bad token, which is a good
thing, but can competing renderers muck with each other's tokens?

[Saman Sami, 2017-04-04 18:25:25]

https://codereview.chromium.org/2789773003/diff/300001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_metadata.mojom (right):

https://codereview.chromium.org/2789773003/diff/300001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_metadata.mojom:37: uint32 frame_token;
On 2017/04/04 18:11:04, Tom Sepez wrote:
> Does it matter that these are guessable?
> It looks like we shoot the renderer if it sends a bad token, which is a good
> thing, but can competing renderers muck with each other's tokens?

Since a RenderWidget cannot send messages to RenderWidgetHostImpl belonging to
another process, I don't believe one renderer can cause problems for another
one. The frame token is basically a way to make distinction between different
frames sent by RenderWidget so in the browser process we can say this set of
messages was meant for frame 1 and these set of messages belong to frame 2.
Another renderer cannot send messages on behalf of another renderer or execute
messages prematurely because it doesn't have access to RenderHidgetHostImpls for
another renderer.

[Saman Sami, 2017-04-04 18:25:39]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:26:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-04-04 18:38:53]

lgtm

[piman, 2017-04-04 19:11:05]

lgtm

[Saman Sami, 2017-04-04 19:43:12]

https://codereview.chromium.org/2789773003/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1661:
ProcessSwapMessages(std::move(queued_messages_.front().second));
OK, maybe this is not a good idea. Maybe it's better not to process messages
coming from a renderer that has exited, because it's kind of equivalent to
receiving messages from a dead process. This might leave the recipient of these
messages in a bad state for example. Please correct me if I'm wrong.

[piman, 2017-04-04 19:49:49]

https://codereview.chromium.org/2789773003/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2789773003/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1661:
ProcessSwapMessages(std::move(queued_messages_.front().second));
On 2017/04/04 19:43:12, Saman Sami wrote:
> OK, maybe this is not a good idea. Maybe it's better not to process messages
> coming from a renderer that has exited, because it's kind of equivalent to
> receiving messages from a dead process. This might leave the recipient of
these
> messages in a bad state for example. Please correct me if I'm wrong.

Recipients have to deal with render process dying and never giving them these
messages today, so I think it's ok to not process the message (i.e. today it
would be equivalent to never receiving the frame).

It's also probably ok to process them, if done at the right place (maybe earlier
in this function), but I think you're right that not doing so is safer.

[Saman Sami, 2017-04-04 19:59:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 20:00:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-04-04 20:48:59]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-04-04 22:25:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:25:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-04-05 00:25:15]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 00:25:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 00:30:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 00:30:35]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2017-04-05 00:47:08]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-04-05 00:47:45]

Patchset #19 (id:380001) has been deleted

[commit-bot: I haz the power, 2017-04-05 00:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 01:25:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 01:25:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-04-05 01:30:43]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 01:31:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-04-05 02:39:47]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-04-05 02:40:04]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-04-05 02:40:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
piman@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2789773003/#ps420001
(title: "Fix rebase mistake")

[commit-bot: I haz the power, 2017-04-05 02:40:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 03:05:07]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1491360004068230,
"parent_rev": "f9cecae6218bcc058cd9cf223cea5e24f037f9c2", "commit_rev":
"d8ecdc4a0429d984fde83e08a1bb343af2d6851b"}

[commit-bot: I haz the power, 2017-04-05 03:07:08]

Description was changed from

==========
Send FrameSwapMessageQueue's messages with a separate IPC

We can't put messages inside ViewHostMsg_SwapCompositorFrame once it's
mojoified, so those messages need to be sent separately. In order
to achieve this, I added an integer token to CompositorFrameMetadata
and send the same token with the messages to the browser. The browser
uses the token to match frames and messages and once both the frame and
its messages arrive, the messages are processed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972
==========

to

==========
Send FrameSwapMessageQueue's messages with a separate IPC

We can't put messages inside ViewHostMsg_SwapCompositorFrame once it's
mojoified, so those messages need to be sent separately. In order
to achieve this, I added an integer token to CompositorFrameMetadata
and send the same token with the messages to the browser. The browser
uses the token to match frames and messages and once both the frame and
its messages arrive, the messages are processed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
BUG=704972

Review-Url: https://codereview.chromium.org/2789773003
Cr-Commit-Position: refs/heads/master@{#461962}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8ecdc4a0429d984fde83e08a1bb...
==========

[commit-bot: I haz the power, 2017-04-05 03:07:09]

Committed patchset #20 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/d8ecdc4a0429d984fde83e08a1bb...