[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2740833005
Cr-Commit-Position: refs/heads/master@{#457962}
Committed: https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf108411491f38632cc

[Eric Seckler, 2017-03-10 16:14:17]

Description was changed from

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

to

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-10 16:30:57]

Description was changed from

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotDraw IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-10 16:36:51]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org

[Eric Seckler, 2017-03-10 16:36:52]

Hi Brian,

I'm wondering if there's a sensible way to test that the renderer actually sends
the BeginFrameDidNotDraw message and that its SwapCompositorFrame message
actually contains an ack.

I looked into RenderViewImplTest, but I think that won't quite work for us given
that we receive/send messages on the compositor thread.

Would it make sense to add some sort of browser test for this, and if so, do you
know of a test harness that we could reuse for this? I couldn't find any tests
that send BeginFrame messages to the renderer and look for correct
CompositorFrame responses or alike.

Otherwise, is it fine to leave these aspects untested?

Thanks!
Eric

[Fady Samuel, 2017-03-10 16:39:30]

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

[Fady Samuel, 2017-03-10 16:39:30]

Drive-by: Please update mojoms too!

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
Please update mojoms too!

[danakj, 2017-03-10 16:40:45]

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

[danakj, 2017-03-10 16:40:46]

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
On 2017/03/10 16:39:30, Fady Samuel wrote:
> Please update mojoms too!

Maybe you can write a presubmit, or some comments, to help people (and
reviewers) remember this

[Fady Samuel, 2017-03-10 16:44:53]

On 2017/03/10 16:40:46, danakj wrote:
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> File cc/ipc/cc_param_traits_macros.h (right):
> 
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
> On 2017/03/10 16:39:30, Fady Samuel wrote:
> > Please update mojoms too!
> 
> Maybe you can write a presubmit, or some comments, to help people (and
> reviewers) remember this

Good point. I'll take a look at that on Monday. I just want to wrap what I'm
doing today first.

[Eric Seckler, 2017-03-10 16:48:37]

On 2017/03/10 16:40:46, danakj wrote:
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> File cc/ipc/cc_param_traits_macros.h (right):
> 
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
> On 2017/03/10 16:39:30, Fady Samuel wrote:
> > Please update mojoms too!
> 
> Maybe you can write a presubmit, or some comments, to help people (and
> reviewers) remember this

I was going to update the mojoms when I'm adding code that sends&receives the
new fields/commands. Last time I tried to proactively add things to
param_traits_macros / mojoms, security reviewers asked that they wanted to see
where and how the new things would be transmitted and used...

[Fady Samuel, 2017-03-10 16:49:28]

On 2017/03/10 16:48:37, Eric Seckler wrote:
> On 2017/03/10 16:40:46, danakj wrote:
> >
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> > File cc/ipc/cc_param_traits_macros.h (right):
> > 
> >
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> > cc/ipc/cc_param_traits_macros.h:209:
IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
> > On 2017/03/10 16:39:30, Fady Samuel wrote:
> > > Please update mojoms too!
> > 
> > Maybe you can write a presubmit, or some comments, to help people (and
> > reviewers) remember this
> 
> I was going to update the mojoms when I'm adding code that sends&receives the
> new fields/commands. Last time I tried to proactively add things to
> param_traits_macros / mojoms, security reviewers asked that they wanted to see
> where and how the new things would be transmitted and used...

OK, we should try to update ParamTraits and mojoms together. So can you avoid
the paramtraits change here?

[Eric Seckler, 2017-03-10 16:53:57]

On 2017/03/10 16:49:28, Fady Samuel wrote:
> On 2017/03/10 16:48:37, Eric Seckler wrote:
> > On 2017/03/10 16:40:46, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> > > File cc/ipc/cc_param_traits_macros.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
> > > cc/ipc/cc_param_traits_macros.h:209:
> IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
> > > On 2017/03/10 16:39:30, Fady Samuel wrote:
> > > > Please update mojoms too!
> > > 
> > > Maybe you can write a presubmit, or some comments, to help people (and
> > > reviewers) remember this
> > 
> > I was going to update the mojoms when I'm adding code that sends&receives
the
> > new fields/commands. Last time I tried to proactively add things to
> > param_traits_macros / mojoms, security reviewers asked that they wanted to
see
> > where and how the new things would be transmitted and used...
> 
> OK, we should try to update ParamTraits and mojoms together. So can you avoid
> the paramtraits change here?

Don't need it for the tests at the moment, but I was hoping maybe there is a way
to add a test for exactly that, see my message to Brian :)

[brianderson, 2017-03-13 22:55:39]

I wouldn't worry about integration tests. This code path will be exercised
extensively as part of telemetry tests.

Not sure about coverage specific to headless though. Do you have a plan for
end-to-end headless tests?

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:558:
OnBeginFrameDidNotDraw)
Here and elsewhere: Can you call this OnBeginFrameDidNotSwap? In the future it's
possible we split out draw from swap. In such a world, we'd want to send a
DidNotSwap message even if we already drew.

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1871:
view_->OnBeginFrameDidNotDraw(ack);
Since this is conditional, should the state in the View tracking
latest_confirmed_begin_frame_sequence_number_ be tracked here instead?

Or at least initialized in RWHI::SetView?

https://codereview.chromium.org/2740833005/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:23: #include
"cc/output/begin_frame_args.h"
Is this needed?

[Eric Seckler, 2017-03-14 11:13:41]

eseckler@chromium.org changed reviewers:
+ dcheng@chromium.org

[Eric Seckler, 2017-03-14 11:13:42]

+dcheng for cc/ipc.

Brian, Fady: PTAL at comment in rwhva.cc :)

Daniel: I was trying to follow your comment on
https://codereview.chromium.org/2591013004/#ps80001 ("Read() methods should
generally out-of-lined"). I tried to do that for BeginFrameArgs/Ack here, but am
facing a problem during linking, see below. Can you help figure out if/how we
can resolve this (I'm not familiar enough with mojo internals), or if we should
stick with inline definition of the methods. Thank you!

Problem during linking:

[2047/2641] SOLINK ./libblink_platform.so
FAILED: libblink_platform.so libblink_platform.so.TOC
[...]
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/cc/ipc/interfaces/begin_frame_args_struct_traits.o: multiple definition of
'mojo::StructTraits<cc::mojom::BeginFrameAckDataView,
cc::BeginFrameAck>::Read(cc::mojom::BeginFrameAckDataView, cc::BeginFrameAck*)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/cc/ipc/interfaces_blink/begin_frame_args_struct_traits.o: previous
definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/cc/ipc/interfaces/begin_frame_args_struct_traits.o: multiple definition of
'mojo::StructTraits<cc::mojom::BeginFrameArgsDataView,
cc::BeginFrameArgs>::Read(cc::mojom::BeginFrameArgsDataView,
cc::BeginFrameArgs*)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/cc/ipc/interfaces_blink/begin_frame_args_struct_traits.o: previous
definition here

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macr...
cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack)
On 2017/03/10 16:39:30, Fady Samuel wrote:
> Please update mojoms too!

Updated the mojoms.

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:558:
OnBeginFrameDidNotDraw)
On 2017/03/13 22:55:39, brianderson wrote:
> Here and elsewhere: Can you call this OnBeginFrameDidNotSwap? In the future
it's
> possible we split out draw from swap. In such a world, we'd want to send a
> DidNotSwap message even if we already drew.

Done.

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1871:
view_->OnBeginFrameDidNotDraw(ack);
On 2017/03/13 22:55:39, brianderson wrote:
> Since this is conditional, should the state in the View tracking
> latest_confirmed_begin_frame_sequence_number_ be tracked here instead?
> 
> Or at least initialized in RWHI::SetView?

I don't think we can track the lcbfsn in RWHI, since it doesn't know anything
about DFH.

A view is also never "reused", i.e. detached from a RWHI and then re-attached
later. It's always constructed for a single RWHI (a pointer to RWH/RWHI is
provided in constructor). Thus, I don't think resetting the tracking during
RWHI::SetView makes a difference.

It also sounded like Fady wants to get rid of skipping drawing in DFH, so maybe
we can remove the tracking of lcbfsn in the RWHVs soon anyway.

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:975:
begin_frame_source_->DidFinishFrame(this, ack);
+fsamuel, +brianderson:

Regarding the discussion about surface synchronization, here's what I think
might be a problem in the approach here:

DFH::SwapDelegatedFrame submits the CompositorFrame to the Surface(Factory),
where it may get stuck in pending state. DisplayScheduler won't see the surface
damage until it becomes active.

The BeginFrameAck, however, is sent separately (this LOC) so will be visible to
DisplayScheduler immediately. When we change DisplayScheduler to using these
acks for deciding when to draw&swap, it might try to draw&swap before the
pending CompositorFrame becomes active (i.e. before the damage is actually on
the surface corresponding to the BeginFrameAck).

Is that a problem?

https://codereview.chromium.org/2740833005/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:23: #include
"cc/output/begin_frame_args.h"
On 2017/03/13 22:55:39, brianderson wrote:
> Is this needed?

Reverted.

[Fady Samuel, 2017-03-14 11:31:38]

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:975:
begin_frame_source_->DidFinishFrame(this, ack);
On 2017/03/14 11:13:41, Eric Seckler wrote:
> +fsamuel, +brianderson:
> 
> Regarding the discussion about surface synchronization, here's what I think
> might be a problem in the approach here:
> 
> DFH::SwapDelegatedFrame submits the CompositorFrame to the Surface(Factory),
> where it may get stuck in pending state. DisplayScheduler won't see the
surface
> damage until it becomes active.
> 
> The BeginFrameAck, however, is sent separately (this LOC) so will be visible
to
> DisplayScheduler immediately. When we change DisplayScheduler to using these
> acks for deciding when to draw&swap, it might try to draw&swap before the
> pending CompositorFrame becomes active (i.e. before the damage is actually on
> the surface corresponding to the BeginFrameAck).
> 
> Is that a problem?

Could we do this from Surface::ActivateFrame then, instead?

[Eric Seckler, 2017-03-14 11:41:55]

On 2017/03/14 11:31:38, Fady Samuel wrote:
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_aura.cc:975:
> begin_frame_source_->DidFinishFrame(this, ack);
> On 2017/03/14 11:13:41, Eric Seckler wrote:
> > +fsamuel, +brianderson:
> > 
> > Regarding the discussion about surface synchronization, here's what I think
> > might be a problem in the approach here:
> > 
> > DFH::SwapDelegatedFrame submits the CompositorFrame to the Surface(Factory),
> > where it may get stuck in pending state. DisplayScheduler won't see the
> surface
> > damage until it becomes active.
> > 
> > The BeginFrameAck, however, is sent separately (this LOC) so will be visible
> to
> > DisplayScheduler immediately. When we change DisplayScheduler to using these
> > acks for deciding when to draw&swap, it might try to draw&swap before the
> > pending CompositorFrame becomes active (i.e. before the damage is actually
on
> > the surface corresponding to the BeginFrameAck).
> > 
> > Is that a problem?
> 
> Could we do this from Surface::ActivateFrame then, instead?

Since the Surface isn't the BeginFrameObserver, it's a bit more tricky than
simply calling DidFinishFrame from there, I think. I'm also not sure if we'd
also have to consider BeginFrameDidNotSwap-acks: If a prior CompositorFrame is
still pending in the surface, would we need to adjust the
latest_confirmed_sequence_number of said ACK?

[Eric Seckler, 2017-03-14 12:46:58]

On 2017/03/14 11:41:55, Eric Seckler wrote:
> On 2017/03/14 11:31:38, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:975:
> > begin_frame_source_->DidFinishFrame(this, ack);
> > On 2017/03/14 11:13:41, Eric Seckler wrote:
> > > +fsamuel, +brianderson:
> > > 
> > > Regarding the discussion about surface synchronization, here's what I
think
> > > might be a problem in the approach here:
> > > 
> > > DFH::SwapDelegatedFrame submits the CompositorFrame to the
Surface(Factory),
> > > where it may get stuck in pending state. DisplayScheduler won't see the
> > surface
> > > damage until it becomes active.
> > > 
> > > The BeginFrameAck, however, is sent separately (this LOC) so will be
visible
> > to
> > > DisplayScheduler immediately. When we change DisplayScheduler to using
these
> > > acks for deciding when to draw&swap, it might try to draw&swap before the
> > > pending CompositorFrame becomes active (i.e. before the damage is actually
> on
> > > the surface corresponding to the BeginFrameAck).
> > > 
> > > Is that a problem?
> > 
> > Could we do this from Surface::ActivateFrame then, instead?
> 
> Since the Surface isn't the BeginFrameObserver, it's a bit more tricky than
> simply calling DidFinishFrame from there, I think. I'm also not sure if we'd
> also have to consider BeginFrameDidNotSwap-acks: If a prior CompositorFrame is
> still pending in the surface, would we need to adjust the
> latest_confirmed_sequence_number of said ACK?

Chatted to Fady about this.

It looks like the interface used between CompositorFrameSinkSupport and its
Client (DFH in this case) is slightly broken - it should use
SubmitCompositorFrame / BeginFrameDidNotSwap, too. I'll prepare a patch.

When that's fixed, we should be able to enforce:
1) Every BeginFrameObserver uses DidFinishFrame
2) All CFSSupportClients use public BFDidNotSwap / SubmitCompositorFrame
3) CFSSupport / Surface will then have to deal with pending CompositorFrames to
update latest_confirmed_sequence_number that the CFSSupport acknowledges.

I'll look into how we can accomplish 3), too.

[Fady Samuel, 2017-03-14 12:48:46]

On 2017/03/14 12:46:58, Eric Seckler wrote:
> On 2017/03/14 11:41:55, Eric Seckler wrote:
> > On 2017/03/14 11:31:38, Fady Samuel wrote:
> > >
> >
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> > > File content/browser/renderer_host/render_widget_host_view_aura.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_ho...
> > > content/browser/renderer_host/render_widget_host_view_aura.cc:975:
> > > begin_frame_source_->DidFinishFrame(this, ack);
> > > On 2017/03/14 11:13:41, Eric Seckler wrote:
> > > > +fsamuel, +brianderson:
> > > > 
> > > > Regarding the discussion about surface synchronization, here's what I
> think
> > > > might be a problem in the approach here:
> > > > 
> > > > DFH::SwapDelegatedFrame submits the CompositorFrame to the
> Surface(Factory),
> > > > where it may get stuck in pending state. DisplayScheduler won't see the
> > > surface
> > > > damage until it becomes active.
> > > > 
> > > > The BeginFrameAck, however, is sent separately (this LOC) so will be
> visible
> > > to
> > > > DisplayScheduler immediately. When we change DisplayScheduler to using
> these
> > > > acks for deciding when to draw&swap, it might try to draw&swap before
the
> > > > pending CompositorFrame becomes active (i.e. before the damage is
actually
> > on
> > > > the surface corresponding to the BeginFrameAck).
> > > > 
> > > > Is that a problem?
> > > 
> > > Could we do this from Surface::ActivateFrame then, instead?
> > 
> > Since the Surface isn't the BeginFrameObserver, it's a bit more tricky than
> > simply calling DidFinishFrame from there, I think. I'm also not sure if we'd
> > also have to consider BeginFrameDidNotSwap-acks: If a prior CompositorFrame
is
> > still pending in the surface, would we need to adjust the
> > latest_confirmed_sequence_number of said ACK?
> 
> Chatted to Fady about this.
> 
> It looks like the interface used between CompositorFrameSinkSupport and its
> Client (DFH in this case) is slightly broken - it should use
> SubmitCompositorFrame / BeginFrameDidNotSwap, too. I'll prepare a patch.
> 
> When that's fixed, we should be able to enforce:
> 1) Every BeginFrameObserver uses DidFinishFrame
> 2) All CFSSupportClients use public BFDidNotSwap / SubmitCompositorFrame
> 3) CFSSupport / Surface will then have to deal with pending CompositorFrames
to
> update latest_confirmed_sequence_number that the CFSSupport acknowledges.
> 
> I'll look into how we can accomplish 3), too.

Thanks Eric for your patience! :-)

[Eric Seckler, 2017-03-14 15:53:00]

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

[Eric Seckler, 2017-03-14 15:53:00]

On 2017/03/14 12:46:58, Eric Seckler wrote:
> It looks like the interface used between CompositorFrameSinkSupport and its
> Client (DFH in this case) is slightly broken - it should use
> SubmitCompositorFrame / BeginFrameDidNotSwap, too. I'll prepare a patch.

I've prepared a patch for this, https://codereview.chromium.org/2755463002/, but
it does require that all CompositorFrame submitters (such as RWHVAura here) set
or forward a valid BeginFrameAck in the CompositorFrame. I think I'll go ahead
and port these clients first before trying to submit said patch.

[danakj, 2017-03-14 15:53:39]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[Eric Seckler, 2017-03-14 16:02:49]

eseckler@chromium.org changed reviewers:
+ danakj@chromium.org, piman@chromium.org
- dcheng@chromium.org

[Eric Seckler, 2017-03-14 16:02:49]

+tsepez@ for cc/ipc

Fady mentioned that Tom is likely more familar with cc/mus-related IPCs. Tom, do
you have an idea how to solve the compilation issue described in my earlier
comment? Otherwise, I'll define the StructTraits::Read methods inline again.

+piman@ for content/ changes.

Thanks!

[Tom Sepez, 2017-03-14 16:38:07]

On 2017/03/14 16:02:49, Eric Seckler wrote:
> +tsepez@ for cc/ipc
> 
> Fady mentioned that Tom is likely more familar with cc/mus-related IPCs. Tom,
do
> you have an idea how to solve the compilation issue described in my earlier
> comment? Otherwise, I'll define the StructTraits::Read methods inline again.
> 
> +piman@ for content/ changes.
> 
> Thanks!

Hmm.  I'm not really sure what purpose the generated cc/ipc/interfaces_blink/
file serves vs. the standard cc/ipc/interfaces/ one, which is the source of the
conflict ...

[dcheng, 2017-03-14 16:41:17]

On 2017/03/14 16:02:49, Eric Seckler wrote:
> +tsepez@ for cc/ipc
> 
> Fady mentioned that Tom is likely more familar with cc/mus-related IPCs. Tom,
do
> you have an idea how to solve the compilation issue described in my earlier
> comment? Otherwise, I'll define the StructTraits::Read methods inline again.
> 
> +piman@ for content/ changes.
> 
> Thanks!

For your compilation issue, see https://codereview.chromium.org/2738683002/ and
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/4hiMy4gRyMI
for some context. You probably need to update how the typemaps are defined for
BeginFrameArgs, which is directly shared between CC and Blink.

[piman, 2017-03-14 20:06:44]

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:180:
IPC_STRUCT_TRAITS_MEMBER(latest_confirmed_sequence_number)
Are there assumptions about this? E.g. I assume it would be invalid for the
latest_confirmed_sequence_number to be more than the latest sent BeginFrame
sequence_number? Should there be sanity checks in RenderWidgetHostImpl?

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:182: IPC_STRUCT_TRAITS_MEMBER(remaining_frames)
Ditto here, any assumptions relative to
sequence_number/latest_confirmed_sequence_number/last sent BeginFrame
sequence_number?

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:183: IPC_STRUCT_TRAITS_MEMBER(has_damage)
Is it necessary to send |has_damage| over IPC? It seems redundant with the info
in the CompositorFrame, and I worry about privileged code assuming consistency
between both, whereas a compromised/malicious renderer could generate
inconsistent data.

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:964: if
(delegated_frame_host_->skipped_frames()) {
delegated_frame_host_ may not exist (e.g. if IsMus() is true, we don't create it
- see other checks in this file).

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:972: }
Could it be useful to move this logic to DelegatedFrameHost, so that it also
applies to Mac?

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:983: if
(delegated_frame_host_->skipped_frames()) {
ditto, delegated_frame_host_ may not exist

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.cc:77: Send(new
ViewHostMsg_BeginFrameDidNotSwap(routing_id_, ack));
So, there are a few cases where we bundle IPC messages with
ViewHostMsg_SwapCompositorFrame, so that they arrive consistently with the frame
and we avoid transient states - see RenderWidget::QueueMessage. This is used for
example for FrameHostMsg_VisualStateResponse, ViewHostMsg_DidFirstPaintAfterLoad
or ViewHostMsg_DidFirstVisuallyNonEmptyPaint.

There is logic to make sure those messages are sent even if the compositor
doesn't generate a frame, e.g. if we canceled the commit because of no damage.

What I'm wondering is if we should do the same thing in that case as we do for
ViewHostMsg_SwapCompositorFrame, that is, bundle those queued messages together
with ViewHostMsg_BeginFrameDidNotSwap in the case we didn't do a frame, to
provide the same consistency property. It would also reduce the number of
IPCs/thread hops.

[danakj, 2017-03-14 20:08:03]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[Eric Seckler, 2017-03-15 11:18:42]

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

[commit-bot: I haz the power, 2017-03-15 11:18:57]

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

[Eric Seckler, 2017-03-15 11:56:48]

Brian, Antoine, PTAL :)

On 2017/03/14 16:41:17, dcheng wrote:
> For your compilation issue, see https://codereview.chromium.org/2738683002/
and
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/4hiMy4gRyMI
> for some context. You probably need to update how the typemaps are defined for
> BeginFrameArgs, which is directly shared between CC and Blink.

OK, I'm confused :) I tried figuring out what is done in other places where
blink and cc share structs, and there seem to be two "ways" this is done, here
examples:

(a) The StructTraits<>::Read for cc::RenderPass is defined in
render_pass_struct_traits.cc. The header render_pass_struct_traits.h is added to
compositor_frame_for_blink.typemap, which is included in
//third_party/WebKit/public/blink_typemaps.gni. The actual typemap for
render_pass (render_pass.typemap) is not included in
//third_party/WebKit/public/blink_typemaps.gni.

(b) The StructTraits<>::Read for cc::SurfaceId is defined in the header
surface_id_struct_traits.h. The surface_id.typemap is directly included in
//third_party/WebKit/public/blink_typemaps.gni.

Of these two, only (b) seems to be used for all classes/headers that are
directly used/included by blink code, such as SurfaceId and BeginFrameArgs. (a)
doesn't work for those, because the typemap will be missing.

Do I have to define a new begin_frame_args_for_blink.typemap if I want to define
StructTraits<>::Read for BeginFrameArgs in the .cc file, but directly use
BeginFrameArgs from blink code (which is the case e.g. in
OffscreenCanvasFrameDispatcher)?

Given that most of the code in cc/ipc doesn't do that, and defines
StructTraits<..>::Read inline, should we stick with that here, too?

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:180:
IPC_STRUCT_TRAITS_MEMBER(latest_confirmed_sequence_number)
On 2017/03/14 20:06:44, piman wrote:
> Are there assumptions about this? E.g. I assume it would be invalid for the
> latest_confirmed_sequence_number to be more than the latest sent BeginFrame
> sequence_number? Should there be sanity checks in RenderWidgetHostImpl?

Since BeginFrameAcks can generally be sent asynchronously to the compositor, we
check for consistency / relevancy when we inspect the acks in the compositor. We
do this in BeginFrameObserverAckTracker and throw away any acks that aren't for
the current BeginFrame's sequence_number:
https://cs.chromium.org/chromium/src/cc/scheduler/begin_frame_source.cc?type=...

Given that, I'm not sure if we need to add more checks for these constraints in
the IPC receivers (RWHViews in this case)? Added a comment to
BeginFrameSource::DidFinishFrame() that the source shouldn't assume that the ack
is for a valid/current/past BeginFrame (which all implementors already do).

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:182: IPC_STRUCT_TRAITS_MEMBER(remaining_frames)
On 2017/03/14 20:06:44, piman wrote:
> Ditto here, any assumptions relative to
> sequence_number/latest_confirmed_sequence_number/last sent BeginFrame
> sequence_number?

remaining_frames is a remnant that doesn't have a purpose anymore - it's always
0 AFAIK. Removing from the struct traits / mojom. There's already a TODO in
BeginFrameAck that we should get rid of it completely (it's a little less
straight forward b/c cc::BackToBackBeginFrameSource needs to be changed for
this).

https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:183: IPC_STRUCT_TRAITS_MEMBER(has_damage)
On 2017/03/14 20:06:44, piman wrote:
> Is it necessary to send |has_damage| over IPC? It seems redundant with the
info
> in the CompositorFrame, and I worry about privileged code assuming consistency
> between both, whereas a compromised/malicious renderer could generate
> inconsistent data.

Yes, it's not necessary - we can set it when we receive the SwapCompositorFrame
/ BeginFrameDidNotSwap IPCs. Removed from struct traits / mojom.

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:964: if
(delegated_frame_host_->skipped_frames()) {
On 2017/03/14 20:06:44, piman wrote:
> delegated_frame_host_ may not exist (e.g. if IsMus() is true, we don't create
it
> - see other checks in this file).

Code moved, checks aren't necessary anymore.

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:972: }
On 2017/03/14 20:06:44, piman wrote:
> Could it be useful to move this logic to DelegatedFrameHost, so that it also
> applies to Mac?

Sounds good, done. I'll add the actual calls to DidFinishFrame in RWHVMac in a
separate patch.

https://codereview.chromium.org/2740833005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:983: if
(delegated_frame_host_->skipped_frames()) {
On 2017/03/14 20:06:44, piman wrote:
> ditto, delegated_frame_host_ may not exist

Gone, too.

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.cc:77: Send(new
ViewHostMsg_BeginFrameDidNotSwap(routing_id_, ack));
On 2017/03/14 20:06:44, piman wrote:
> So, there are a few cases where we bundle IPC messages with
> ViewHostMsg_SwapCompositorFrame, so that they arrive consistently with the
frame
> and we avoid transient states - see RenderWidget::QueueMessage. This is used
for
> example for FrameHostMsg_VisualStateResponse,
ViewHostMsg_DidFirstPaintAfterLoad
> or ViewHostMsg_DidFirstVisuallyNonEmptyPaint.
> 
> There is logic to make sure those messages are sent even if the compositor
> doesn't generate a frame, e.g. if we canceled the commit because of no damage.
> 
> What I'm wondering is if we should do the same thing in that case as we do for
> ViewHostMsg_SwapCompositorFrame, that is, bundle those queued messages
together
> with ViewHostMsg_BeginFrameDidNotSwap in the case we didn't do a frame, to
> provide the same consistency property. It would also reduce the number of
> IPCs/thread hops.

This sounds like an interesting optimization, but it's probably not
straight-forward :)

One question on my mind would be whether breaking the SwapPromise for the queued
messages is actually always accompanied by a BeginFrameDidNotSwap (for the
current BeginFrame), or if they might also be broken while a CompositorFrame is
produced by the impl thread (b/c of animations and such).

In either case, we'd have to bring the queued messages from the broken
SwapPromise into the impl thread (if they didn't get there yet) and then attach
them to the next BeginFrameDidNotSwap (or SwapCompositorFrame) IPC, right?

Would it make sense to / would we be able to do something similar for
MojoCompositorFrameSink communication?

Anyway, shall we explore this separately from this change? :)

[Eric Seckler, 2017-03-15 12:04:29]

Description was changed from

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotDraw IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-03-15 12:51:20]

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

[commit-bot: I haz the power, 2017-03-15 12:51:22]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-03-15 17:26:57]

Patchset #5 (id:80001) has been deleted

[brianderson, 2017-03-15 18:54:23]

lgtm

[dcheng, 2017-03-15 20:10:47]

On 2017/03/15 11:56:48, Eric Seckler wrote:
> Brian, Antoine, PTAL :)
> 
> On 2017/03/14 16:41:17, dcheng wrote:
> > For your compilation issue, see https://codereview.chromium.org/2738683002/
> and
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/4hiMy4gRyMI
> > for some context. You probably need to update how the typemaps are defined
for
> > BeginFrameArgs, which is directly shared between CC and Blink.
> 
> OK, I'm confused :) I tried figuring out what is done in other places where
> blink and cc share structs, and there seem to be two "ways" this is done, here
> examples:
> 
> (a) The StructTraits<>::Read for cc::RenderPass is defined in
> render_pass_struct_traits.cc. The header render_pass_struct_traits.h is added
to
> compositor_frame_for_blink.typemap, which is included in
> //third_party/WebKit/public/blink_typemaps.gni. The actual typemap for
> render_pass (render_pass.typemap) is not included in
> //third_party/WebKit/public/blink_typemaps.gni.
> 
> (b) The StructTraits<>::Read for cc::SurfaceId is defined in the header
> surface_id_struct_traits.h. The surface_id.typemap is directly included in
> //third_party/WebKit/public/blink_typemaps.gni.
> 
> Of these two, only (b) seems to be used for all classes/headers that are
> directly used/included by blink code, such as SurfaceId and BeginFrameArgs.
(a)
> doesn't work for those, because the typemap will be missing.
> 
> Do I have to define a new begin_frame_args_for_blink.typemap if I want to
define
> StructTraits<>::Read for BeginFrameArgs in the .cc file, but directly use
> BeginFrameArgs from blink code (which is the case e.g. in
> OffscreenCanvasFrameDispatcher)?

Yes, this is the best way forward for now. yzshen@ suggested an alternative
which I'll investigate later and switch over to using if it works everywhere.

> 
> Given that most of the code in cc/ipc doesn't do that, and defines
> StructTraits<..>::Read inline, should we stick with that here, too?

The fact that most cc/ipc code doesn't out-of-line the Read() method is an
oversight. We generally prefer to out-of-line Read() to avoid excessive
inlining, as Read() is generally non-trivial.

[piman, 2017-03-15 21:17:10]

Mostly good. Main thing is the question about the validity of the DCHECK
assuming untrusted input.

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2740833005/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.cc:77: Send(new
ViewHostMsg_BeginFrameDidNotSwap(routing_id_, ack));
On 2017/03/15 11:56:48, Eric Seckler wrote:
> On 2017/03/14 20:06:44, piman wrote:
> > So, there are a few cases where we bundle IPC messages with
> > ViewHostMsg_SwapCompositorFrame, so that they arrive consistently with the
> frame
> > and we avoid transient states - see RenderWidget::QueueMessage. This is used
> for
> > example for FrameHostMsg_VisualStateResponse,
> ViewHostMsg_DidFirstPaintAfterLoad
> > or ViewHostMsg_DidFirstVisuallyNonEmptyPaint.
> > 
> > There is logic to make sure those messages are sent even if the compositor
> > doesn't generate a frame, e.g. if we canceled the commit because of no
damage.
> > 
> > What I'm wondering is if we should do the same thing in that case as we do
for
> > ViewHostMsg_SwapCompositorFrame, that is, bundle those queued messages
> together
> > with ViewHostMsg_BeginFrameDidNotSwap in the case we didn't do a frame, to
> > provide the same consistency property. It would also reduce the number of
> > IPCs/thread hops.
> 
> This sounds like an interesting optimization, but it's probably not
> straight-forward :)
> 
> One question on my mind would be whether breaking the SwapPromise for the
queued
> messages is actually always accompanied by a BeginFrameDidNotSwap (for the
> current BeginFrame), or if they might also be broken while a CompositorFrame
is
> produced by the impl thread (b/c of animations and such).

I think ideally if the compositor is making frames after the commit (that was
canceled because e.g. no updates), we ought to queue the messages with those
frames. Which we probably don't do today.

> In either case, we'd have to bring the queued messages from the broken
> SwapPromise into the impl thread (if they didn't get there yet) and then
attach
> them to the next BeginFrameDidNotSwap (or SwapCompositorFrame) IPC, right?

Yes I think that's the idea

> Would it make sense to / would we be able to do something similar for
> MojoCompositorFrameSink communication?

We talked about this with fsamuel and others about the need to have a system to
support all this in the new world because those IPCs don't make a lot of sense
to send directly to the DisplayCompositor. See some discussion here:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-mojo/sy4ASAZr0b...

> Anyway, shall we explore this separately from this change? :)

Sure, but may be worth a todo / looping back to a bug

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:941:
frame.metadata.begin_frame_ack.has_damage = true;
nit: you could do this in RenderWidgetHostImpl, so that it applies to all
platforms. Arguably same with the checks below.

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:943:
DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
What makes this DCHECK true, if ack comes (after a copy) from an untrusted
renderer?
We can have that DCHECK on the sender side, but maybe we need to refuse/discard
the message if it doesn't satisfy this condition?

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:960:
DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
Ditto here.

[Eric Seckler, 2017-03-16 10:24:13]

eseckler@chromium.org changed reviewers:
+ asvitkine@chromium.org, dcheng@chromium.org
- tsepez@chromium.org

[Eric Seckler, 2017-03-16 10:24:15]

Thanks, all addressed!

Daniel, could you have a look at ipc security including
    third_party/WebKit/public/blink_typemaps.gni

+asvitkine@ for:
    tools/metrics/histograms/histograms.xml

On 2017/03/15 20:10:47, dcheng wrote:
> > Do I have to define a new begin_frame_args_for_blink.typemap if I want to
> define
> > StructTraits<>::Read for BeginFrameArgs in the .cc file, but directly use
> > BeginFrameArgs from blink code (which is the case e.g. in
> > OffscreenCanvasFrameDispatcher)?
> 
> Yes, this is the best way forward for now. yzshen@ suggested an alternative
> which I'll investigate later and switch over to using if it works everywhere.

Done.

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:941:
frame.metadata.begin_frame_ack.has_damage = true;
On 2017/03/15 21:17:10, piman wrote:
> nit: you could do this in RenderWidgetHostImpl, so that it applies to all
> platforms. Arguably same with the checks below.

Moved all to RWHI.

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:943:
DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
On 2017/03/15 21:17:10, piman wrote:
> What makes this DCHECK true, if ack comes (after a copy) from an untrusted
> renderer?
> We can have that DCHECK on the sender side, but maybe we need to
refuse/discard
> the message if it doesn't satisfy this condition?

Replaced it with a ReceivedBadMessage, assuming this is more sensible?

The reason I was simply DCHECKing is that I was under the impression that it's
fine to DCHECK, as opposed to CHECK, for validity (so that bad input is flagged
on the bots) provided that you also safely handle / throw out this input in
production. Since the compositor code ignores bad BeginFrameAcks, that would
have been the case here.

I'm still not entirely clear whether such DCHECKing would be fine, since [1]
only mentions that "DCHECK is acceptable", but doesn't explain this further...

Anyway, I'm also happy with ReceivedBadMessage :)

[1]
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:960:
DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
On 2017/03/15 21:17:10, piman wrote:
> Ditto here.

Done, moved as well.

[Eric Seckler, 2017-03-16 10:26:47]

eseckler@chromium.org changed reviewers:
+ holte@chromium.org
- asvitkine@chromium.org

[Eric Seckler, 2017-03-16 10:26:48]

On 2017/03/16 10:24:15, Eric Seckler wrote:
> +asvitkine@ for:
>     tools/metrics/histograms/histograms.xml

Alexei is "very slow", so giving to holte@ instead.

[Eric Seckler, 2017-03-16 10:27:23]

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

[commit-bot: I haz the power, 2017-03-16 10:27:34]

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

[Eric Seckler, 2017-03-16 10:29:23]

Patchset #7 (id:140001) has been deleted

[Eric Seckler, 2017-03-16 10:29:32]

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

[commit-bot: I haz the power, 2017-03-16 10:29:43]

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-16 10:53:12]

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

[commit-bot: I haz the power, 2017-03-16 10:53:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Eric Seckler, 2017-03-16 11:52:25]

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

[commit-bot: I haz the power, 2017-03-16 11:52:45]

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-16 12:20:46]

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

[commit-bot: I haz the power, 2017-03-16 12:20:47]

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_...)

[Eric Seckler, 2017-03-16 13:31:27]

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

[commit-bot: I haz the power, 2017-03-16 13:31:48]

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

[piman, 2017-03-16 18:32:14]

LGTM + couple of nits.

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2740833005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:943:
DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
On 2017/03/16 10:24:15, Eric Seckler wrote:
> On 2017/03/15 21:17:10, piman wrote:
> > What makes this DCHECK true, if ack comes (after a copy) from an untrusted
> > renderer?
> > We can have that DCHECK on the sender side, but maybe we need to
> refuse/discard
> > the message if it doesn't satisfy this condition?
> 
> Replaced it with a ReceivedBadMessage, assuming this is more sensible?

See comments in RWHI.

> The reason I was simply DCHECKing is that I was under the impression that it's
> fine to DCHECK, as opposed to CHECK, for validity (so that bad input is
flagged
> on the bots) provided that you also safely handle / throw out this input in
> production. Since the compositor code ignores bad BeginFrameAcks, that would
> have been the case here.
> 
> I'm still not entirely clear whether such DCHECKing would be fine, since [1]
> only mentions that "DCHECK is acceptable", but doesn't explain this further...
> 
> Anyway, I'm also happy with ReceivedBadMessage :)
> 
> [1]
>
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

That page explicitly says "Avoid browser side DCHECKs and WebKit ASSERTs that
fail with invalid input from the renderer. Such conditions should be explicitly
checked instead. Otherwise the macros lead to false sense of security since they
aren't compiled into release builds."

From my POV, the DCHECK is actively harmful, because as a reader of the code, I
take the DCHECK as a guarantee that this can't happen. That is, it makes me
think that some other code (e.g. the struct traits) made sure to fail
deserialization if this condition could happen. In turn I may start writing code
that depends on this being true, causing security issues. Separately, the DCHECK
also prevents attempts at testing that the browser does behave correctly if
given bad input, since such a test would assert in debug.

OTOH, when possible, a DCHECK on the sender side is a good thing to have, it
won't prevent security issues, but it can help debugging, because it is closer
to the code that made the mistake, and can even give you a useful stack trace
(this one wouldn't).

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

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1853:
bad_message::RWH_INVALID_BEGIN_FRAME_ACK_COMPOSITOR_FRAME);
nit: ReceivedBadMessage is a bit of a big hammer. It's not necessarily bad per
se, in particular if we believe a misbehaving renderer could cause bad things to
happen in the browser. However it can make debugging hard, if some condition
causes this to happen, which could cause the renderer to be killed by the
browser as one tries to debug it.

In this case, I think just returning false is enough, maybe with a DLOG(ERROR).
If you feel otherwise, I'm ok if you want to leave it.

Either way, could you move this code to maybe l.1835, so that we do all sanity
checking before doing anything "active", to avoid doing parts of the work (e.g.
latency tracker, etc.) if we are going to refuse the message.

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1893: GetProcess(),
bad_message::RWH_INVALID_BEGIN_FRAME_ACK_DID_NOT_SWAP);
Ditto, ReceivedBadMessage may be overkill, and just DLOG + return may be enough.

[Steven Holte, 2017-03-16 20:35:56]

histograms lgtm

[dcheng, 2017-03-17 05:54:03]

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_arg...
File cc/ipc/begin_frame_args_struct_traits.cc (right):

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_arg...
cc/ipc/begin_frame_args_struct_traits.cc:22: out->type =
static_cast<cc::BeginFrameArgs::BeginFrameArgsType>(data.type());
Mind adding a TODO to use EnumTraits for this?

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_metadata_struct_traits.h (right):

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_metadata_struct_traits.h:119: static cc::BeginFrameAck
begin_frame_ack(
Return a const reference here--even though it's a POD, it's still not free to
copy

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

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1853:
bad_message::RWH_INVALID_BEGIN_FRAME_ACK_COMPOSITOR_FRAME);
On 2017/03/16 18:32:14, piman wrote:
> nit: ReceivedBadMessage is a bit of a big hammer. It's not necessarily bad per
> se, in particular if we believe a misbehaving renderer could cause bad things
to
> happen in the browser. However it can make debugging hard, if some condition
> causes this to happen, which could cause the renderer to be killed by the
> browser as one tries to debug it.
> 
> In this case, I think just returning false is enough, maybe with a
DLOG(ERROR).
> If you feel otherwise, I'm ok if you want to leave it.
> 
> Either way, could you move this code to maybe l.1835, so that we do all sanity
> checking before doing anything "active", to avoid doing parts of the work
(e.g.
> latency tracker, etc.) if we are going to refuse the message.

I could see this going either way... with my security hat on, it does seem nice
to strongly enforce invariants are broken, but I also see your point. But if
it's just devs debugging, it's often not going to be an official build anyway,
right? At which point the devs can just patch it out... or patch out the call to
ReceivedBadMessage in the debugger entirely.

I don't feel strongly about this, I just happen to lean slightly the other way
=)

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1900: DCHECK_EQ(0u,
ack.remaining_frames);
Will anything be negatively affected if these DCHECKs aren't true? It looks like
this eventually passes to DelegatedFrameHost, which makes a copy of this and
saves it (with potentially unexpected state in either of these two fields).

[Eric Seckler, 2017-03-17 11:05:58]

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

[commit-bot: I haz the power, 2017-03-17 11:06:12]

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

[Eric Seckler, 2017-03-17 11:06:45]

On 2017/03/16 18:32:14, piman wrote:
> That page explicitly says "Avoid browser side DCHECKs and WebKit ASSERTs that
> fail with invalid input from the renderer. Such conditions should be
explicitly
> checked instead. Otherwise the macros lead to false sense of security since
they
> aren't compiled into release builds."
> 
> From my POV, the DCHECK is actively harmful, because as a reader of the code,
I
> take the DCHECK as a guarantee that this can't happen. That is, it makes me
> think that some other code (e.g. the struct traits) made sure to fail
> deserialization if this condition could happen. In turn I may start writing
code
> that depends on this being true, causing security issues. Separately, the
DCHECK
> also prevents attempts at testing that the browser does behave correctly if
> given bad input, since such a test would assert in debug.
> 
> OTOH, when possible, a DCHECK on the sender side is a good thing to have, it
> won't prevent security issues, but it can help debugging, because it is closer
> to the code that made the mistake, and can even give you a useful stack trace
> (this one wouldn't).

Ah, I didn't read far enough :) Thanks for the explanation, that makes sense!

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_arg...
File cc/ipc/begin_frame_args_struct_traits.cc (right):

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_arg...
cc/ipc/begin_frame_args_struct_traits.cc:22: out->type =
static_cast<cc::BeginFrameArgs::BeginFrameArgsType>(data.type());
On 2017/03/17 05:54:02, dcheng wrote:
> Mind adding a TODO to use EnumTraits for this?

Done.

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_metadata_struct_traits.h (right):

https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_metadata_struct_traits.h:119: static cc::BeginFrameAck
begin_frame_ack(
On 2017/03/17 05:54:02, dcheng wrote:
> Return a const reference here--even though it's a POD, it's still not free to
> copy

Done.

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

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1853:
bad_message::RWH_INVALID_BEGIN_FRAME_ACK_COMPOSITOR_FRAME);
On 2017/03/17 05:54:02, dcheng wrote:
> On 2017/03/16 18:32:14, piman wrote:
> > nit: ReceivedBadMessage is a bit of a big hammer. It's not necessarily bad
per
> > se, in particular if we believe a misbehaving renderer could cause bad
things
> to
> > happen in the browser. However it can make debugging hard, if some condition
> > causes this to happen, which could cause the renderer to be killed by the
> > browser as one tries to debug it.
> > 
> > In this case, I think just returning false is enough, maybe with a
> DLOG(ERROR).
> > If you feel otherwise, I'm ok if you want to leave it.
> > 
> > Either way, could you move this code to maybe l.1835, so that we do all
sanity
> > checking before doing anything "active", to avoid doing parts of the work
> (e.g.
> > latency tracker, etc.) if we are going to refuse the message.
> 
> I could see this going either way... with my security hat on, it does seem
nice
> to strongly enforce invariants are broken, but I also see your point. But if
> it's just devs debugging, it's often not going to be an official build anyway,
> right? At which point the devs can just patch it out... or patch out the call
to
> ReceivedBadMessage in the debugger entirely.
> 
> I don't feel strongly about this, I just happen to lean slightly the other way
> =)

I was pointed to ReceivedBadMessage by mkwst@, who also mentioned that it'd be
fine to simply drop messages if the renderer doesn't expect a response to them.
But if the renderer sends a SwapCompositorFrame message, it also eventually
expects a ReclaimCompositorResources / CompositorFrameAck so would likely get
stuck somewhere down the line (?).

I'm happy either way, but terminating seems like a reasonable & explicit way to
surface that something broke. (Compared to a DLOG of a dropped frame that might
easily be missed?)

I agree that DCHECKs in the senders have better stack traces, as Antoine
mentioned, so I'm adding those, too.

If Antoine still leans towards removing the ReceivedBadMessage given this, I'll
do that. Otherwise let's leave them :)

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1893: GetProcess(),
bad_message::RWH_INVALID_BEGIN_FRAME_ACK_DID_NOT_SWAP);
On 2017/03/16 18:32:14, piman wrote:
> Ditto, ReceivedBadMessage may be overkill, and just DLOG + return may be
enough.

Done.

https://codereview.chromium.org/2740833005/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1900: DCHECK_EQ(0u,
ack.remaining_frames);
On 2017/03/17 05:54:03, dcheng wrote:
> Will anything be negatively affected if these DCHECKs aren't true? It looks
like
> this eventually passes to DelegatedFrameHost, which makes a copy of this and
> saves it (with potentially unexpected state in either of these two fields).

Yeah, they should actually have these values, otherwise things may not work as
expected. Sounds like we should just be forcing these to the correct values here
then, and not rely on default initialization taking care of this? Done, also in
OnSwapCompositorFrame.

[commit-bot: I haz the power, 2017-03-17 13:15:54]

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

[commit-bot: I haz the power, 2017-03-17 13:15:57]

Dry run: This issue passed the CQ dry run.

[piman, 2017-03-17 17:38:09]

LGTM. I don't feel that strongly about ReceivedBadMessage, just that in the past
we had unexplained renderer "crash" spikes without anything in crash/ to explain
it, and it took a while to figure out that it was a renderer bug causing the
browser to kill it - but at the time we were not systematically UMA-logging
those. Now that we are, maybe I'm less concerned (modulo the general lack of
consistency), assuming someone takes a look at UMA when we see renderer crash
spikes.
Any way, this is good with me as is.

[dcheng, 2017-03-17 18:39:19]

LGTM with one thought

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

https://codereview.chromium.org/2740833005/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1839: // |has_damage|
and |remaining_frames| are not transmitted.
Alternatively, this could just be part of the ReceivedBadMessage check on line
1831. Conceptually, that might make more sense.

https://codereview.chromium.org/2740833005/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1898:
modified_ack.remaining_frames = 0;
Ditto.

[piman, 2017-03-17 19:20:55]

On Fri, Mar 17, 2017 at 11:39 AM, <dcheng@chromium.org> wrote:

> LGTM with one thought
>
>
> https://codereview.chromium.org/2740833005/diff/240001/
> content/browser/renderer_host/render_widget_host_impl.cc
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
>
> https://codereview.chromium.org/2740833005/diff/240001/
> content/browser/renderer_host/render_widget_host_impl.cc#newcode1839
> content/browser/renderer_host/render_widget_host_impl.cc:1839: //
> |has_damage| and |remaining_frames| are not transmitted.
> Alternatively, this could just be part of the ReceivedBadMessage check
> on line 1831. Conceptually, that might make more sense.
>

I had Eric remove the redundant values from the IPC protocol, so that we
don't need to trust or check the renderer, and instead give them the
correct values on the receiving side. I think it is better to prevent
accidental use of untrusted values (i.e. those 2 fields never contain
untrusted data).


>
> https://codereview.chromium.org/2740833005/diff/240001/
> content/browser/renderer_host/render_widget_host_impl.cc#newcode1898
> content/browser/renderer_host/render_widget_host_impl.cc:1898:
> modified_ack.remaining_frames = 0;
> Ditto.
>
> https://codereview.chromium.org/2740833005/
>

-- 
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.

[dcheng, 2017-03-17 19:24:40]

On 2017/03/17 19:20:55, piman wrote:
> On Fri, Mar 17, 2017 at 11:39 AM, <mailto:dcheng@chromium.org> wrote:
> 
> > LGTM with one thought
> >
> >
> > https://codereview.chromium.org/2740833005/diff/240001/
> > content/browser/renderer_host/render_widget_host_impl.cc
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> >
> > https://codereview.chromium.org/2740833005/diff/240001/
> > content/browser/renderer_host/render_widget_host_impl.cc#newcode1839
> > content/browser/renderer_host/render_widget_host_impl.cc:1839: //
> > |has_damage| and |remaining_frames| are not transmitted.
> > Alternatively, this could just be part of the ReceivedBadMessage check
> > on line 1831. Conceptually, that might make more sense.
> >
> 
> I had Eric remove the redundant values from the IPC protocol, so that we
> don't need to trust or check the renderer, and instead give them the
> correct values on the receiving side. I think it is better to prevent
> accidental use of untrusted values (i.e. those 2 fields never contain
> untrusted data).

Ah, yeah, that sounds better: I had assumed that was hard for other reasons, but
if we can just not send those fields to begin with, I would prefer that =)

> 
> 
> >
> > https://codereview.chromium.org/2740833005/diff/240001/
> > content/browser/renderer_host/render_widget_host_impl.cc#newcode1898
> > content/browser/renderer_host/render_widget_host_impl.cc:1898:
> > modified_ack.remaining_frames = 0;
> > Ditto.
> >
> > https://codereview.chromium.org/2740833005/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[Eric Seckler, 2017-03-18 06:04:06]

On 2017/03/17 19:24:40, dcheng wrote:
> On 2017/03/17 19:20:55, piman wrote:
> > On Fri, Mar 17, 2017 at 11:39 AM, <mailto:dcheng@chromium.org> wrote:
> > 
> > > LGTM with one thought
> > >
> > >
> > > https://codereview.chromium.org/2740833005/diff/240001/
> > > content/browser/renderer_host/render_widget_host_impl.cc
> > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > >
> > > https://codereview.chromium.org/2740833005/diff/240001/
> > > content/browser/renderer_host/render_widget_host_impl.cc#newcode1839
> > > content/browser/renderer_host/render_widget_host_impl.cc:1839: //
> > > |has_damage| and |remaining_frames| are not transmitted.
> > > Alternatively, this could just be part of the ReceivedBadMessage check
> > > on line 1831. Conceptually, that might make more sense.
> > >
> > 
> > I had Eric remove the redundant values from the IPC protocol, so that we
> > don't need to trust or check the renderer, and instead give them the
> > correct values on the receiving side. I think it is better to prevent
> > accidental use of untrusted values (i.e. those 2 fields never contain
> > untrusted data).
> 
> Ah, yeah, that sounds better: I had assumed that was hard for other reasons,
but
> if we can just not send those fields to begin with, I would prefer that =)

Yep, that's what we do and why I was using dchecks for these fields originally.
I'm gonna leave things as-is, since forcing the value seems more robust to
future changes.

> > 
> > 
> > >
> > > https://codereview.chromium.org/2740833005/diff/240001/
> > > content/browser/renderer_host/render_widget_host_impl.cc#newcode1898
> > > content/browser/renderer_host/render_widget_host_impl.cc:1898:
> > > modified_ack.remaining_frames = 0;
> > > Ditto.
> > >
> > > https://codereview.chromium.org/2740833005/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[Eric Seckler, 2017-03-18 06:04:17]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-03-18 06:04:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2740833005/#ps240001
(title: "address security comments")

[commit-bot: I haz the power, 2017-03-18 06:04:29]

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-18 06:10:21]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489817057341040,
"parent_rev": "7f38e48ab4ee07afa49a7d415296b7db3fc14e06", "commit_rev":
"31ef6f5c55de6abf3e1c7cf108411491f38632cc"}

[commit-bot: I haz the power, 2017-03-18 06:11:49]

Description was changed from

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

This patch adds forwarding of BeginFrameAcks from the renderer through
the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser,
where they are received and passed on by RenderWidgetHostViewAura.

Other platforms to follow.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2740833005
Cr-Commit-Position: refs/heads/master@{#457962}
Committed:
https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf10841...
==========

[commit-bot: I haz the power, 2017-03-18 06:11:52]

Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf10841...

[dcheng, 2017-03-18 23:14:02]

On 2017/03/18 06:04:06, Eric Seckler wrote:
> On 2017/03/17 19:24:40, dcheng wrote:
> > On 2017/03/17 19:20:55, piman wrote:
> > > On Fri, Mar 17, 2017 at 11:39 AM, <mailto:dcheng@chromium.org> wrote:
> > > 
> > > > LGTM with one thought
> > > >
> > > >
> > > > https://codereview.chromium.org/2740833005/diff/240001/
> > > > content/browser/renderer_host/render_widget_host_impl.cc
> > > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > > >
> > > > https://codereview.chromium.org/2740833005/diff/240001/
> > > > content/browser/renderer_host/render_widget_host_impl.cc#newcode1839
> > > > content/browser/renderer_host/render_widget_host_impl.cc:1839: //
> > > > |has_damage| and |remaining_frames| are not transmitted.
> > > > Alternatively, this could just be part of the ReceivedBadMessage check
> > > > on line 1831. Conceptually, that might make more sense.
> > > >
> > > 
> > > I had Eric remove the redundant values from the IPC protocol, so that we
> > > don't need to trust or check the renderer, and instead give them the
> > > correct values on the receiving side. I think it is better to prevent
> > > accidental use of untrusted values (i.e. those 2 fields never contain
> > > untrusted data).
> > 
> > Ah, yeah, that sounds better: I had assumed that was hard for other reasons,
> but
> > if we can just not send those fields to begin with, I would prefer that =)
> 
> Yep, that's what we do and why I was using dchecks for these fields
originally.
> I'm gonna leave things as-is, since forcing the value seems more robust to
> future changes.

Hmm, I may be missing something, but I thought piman's suggestion was to just
not send the other fields: in other words, don't send across an entire
BeginFrameAck, but just send across the sequence number?

> 
> > > 
> > > 
> > > >
> > > > https://codereview.chromium.org/2740833005/diff/240001/
> > > > content/browser/renderer_host/render_widget_host_impl.cc#newcode1898
> > > > content/browser/renderer_host/render_widget_host_impl.cc:1898:
> > > > modified_ack.remaining_frames = 0;
> > > > Ditto.
> > > >
> > > > https://codereview.chromium.org/2740833005/
> > > >
> > > 
> > > -- 
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[Eric Seckler, 2017-03-19 06:49:32]

On 2017/03/18 23:14:02, dcheng wrote:
> Hmm, I may be missing something, but I thought piman's suggestion was to just
> not send the other fields: in other words, don't send across an entire
> BeginFrameAck, but just send across the sequence number?

We've explicitly left those fields out of the mojom and param_traits_macros so
that they are not transmitted - see comments on patch set 2.