Remove GpuHostMsg_FrameDrawn and replace with client channel swap ack

Remove GpuHostMsg_FrameDrawn and replace with client channel swap ack

This also makes it unnecessary to signal a sync point
callback for swap completion for browser
compositor surfaces which is skipped now.

BUG=417945
Committed: https://crrev.com/df212b3be96421b8b0bcce4947a50236abc38615
Cr-Commit-Position: refs/heads/master@{#298972}

[no sievers, 2014-10-07 01:00:17]

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

[no sievers, 2014-10-07 01:01:38]

ptal

depends on
https://codereview.chromium.org/632113002/ for removing the SetLatencyInfo call
site in the renderer
https://codereview.chromium.org/629293004/ for figuring out how
SoftwareBrowserCompositorOutputSurface should work (then I could move
OnSwapBuffersCompleted(latency_info) to the BrowserCompositorOutputSurface base
class and share it).

[piman, 2014-10-07 02:30:39]

https://codereview.chromium.org/628703005/diff/1/content/browser/compositor/g...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/628703005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:43:
base::Unretained(this)));
Because ContextProvider is refcounted, it might outlive the
GpuBrowserCompositorOutputSurface, and so the context might receive a callback
after the GpuBrowserCompositorOutputSurface was destroyed.
Should we use a weak pointer, or provide a mechanism to cancel the callbacks?

https://codereview.chromium.org/628703005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:85:
base::Unretained(this)));
Same concern here wrt lifetime.

https://codereview.chromium.org/628703005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:86:
context_provider_->ContextSupport()->Swap();
nit: add a DCHECK that the frame is a full frame? So that we don't try to enable
partial swaps and wonder why things don't work.

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.cc:515:
swap_buffers_completion_callback_.Reset();
If we have more than 1 frame deep, I don't think this will work, because we may
call a second SetLatencyInfo+SwapBuffers before we got the first ack, and so the
second callback will never fire.

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:116: void
ImageTransportHelper::SendLatencyInfo(
nit: maybe we should rename this as SwapBuffersCompleted or something. It's not
clear without looking at PassThroughImageTransportSurface why this is the
swapbuffers ack.

[Mike West, 2014-10-07 04:46:22]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org, tsepez@chromium.org

[Mike West, 2014-10-07 04:46:23]

Drive-by IPC comment. +tsepez@ to eventually give an OWNERS stamp.

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.cc:512: const
std::vector<ui::LatencyInfo>& latency_info) {
It doesn't look like you have any limits on the amount of data that can be sent
back and forth over this message. It seems like calling LatencyInfo::Verify on
this vector would be a reasonable way of enforcing sanity checks. WDYT?

[no sievers, 2014-10-07 19:47:24]

https://codereview.chromium.org/628703005/diff/1/content/browser/compositor/g...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/628703005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:43:
base::Unretained(this)));
On 2014/10/07 02:30:39, piman (Very slow to review) wrote:
> Because ContextProvider is refcounted, it might outlive the
> GpuBrowserCompositorOutputSurface, and so the context might receive a callback
> after the GpuBrowserCompositorOutputSurface was destroyed.
> Should we use a weak pointer, or provide a mechanism to cancel the callbacks?

Done. I've used CancelableCallback (which essentially wraps a WeakPtr) just to
discourage more weakptr bindings a bit. Also the base class already has a
WeakPtr factory so it can get a bit confusing.

https://codereview.chromium.org/628703005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:86:
context_provider_->ContextSupport()->Swap();
On 2014/10/07 02:30:39, piman (Very slow to review) wrote:
> nit: add a DCHECK that the frame is a full frame? So that we don't try to
enable
> partial swaps and wonder why things don't work.

Done.

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.cc:515:
swap_buffers_completion_callback_.Reset();
On 2014/10/07 02:30:39, piman (Very slow to review) wrote:
> If we have more than 1 frame deep, I don't think this will work, because we
may
> call a second SetLatencyInfo+SwapBuffers before we got the first ack, and so
the
> second callback will never fire.

Done.

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/628703005/diff/1/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface.cc:116: void
ImageTransportHelper::SendLatencyInfo(
On 2014/10/07 02:30:39, piman (Very slow to review) wrote:
> nit: maybe we should rename this as SwapBuffersCompleted or something. It's
not
> clear without looking at PassThroughImageTransportSurface why this is the
> swapbuffers ack.

Done.

[no sievers, 2014-10-07 19:52:30]

On 2014/10/07 04:46:23, Mike West wrote:
> Drive-by IPC comment. +tsepez@ to eventually give an OWNERS stamp.
> 
>
https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/628703005/diff/1/content/common/gpu/client/co...
> content/common/gpu/client/command_buffer_proxy_impl.cc:512: const
> std::vector<ui::LatencyInfo>& latency_info) {
> It doesn't look like you have any limits on the amount of data that can be
sent
> back and forth over this message. It seems like calling LatencyInfo::Verify on
> this vector would be a reasonable way of enforcing sanity checks. WDYT?

Thanks for catching that! I have updated the patch to match the verification as
it exists today (gpu_process_host_ui_shim.cc).

Do we mostly care about RenderWidgetHostImpl::CompositorFrameDrawn() iterating
through all those elements with a malicious renderer?
What about all the allocations and copying we go through? Should we really be
overriding the IPC traits for vector<LatencyInfo> so that it limits the size
there?

Should we also check this earlier in DelegatedFrameHost (and RWHVAndroid) when
we receive the original info from the renderer, since I assume we trust the
renderer less than the gpu process.

[piman, 2014-10-07 20:17:06]

On Tue, Oct 7, 2014 at 12:52 PM, <sievers@chromium.org> wrote:

> On 2014/10/07 04:46:23, Mike West wrote:
>
>> Drive-by IPC comment. +tsepez@ to eventually give an OWNERS stamp.
>>
>
>
> https://codereview.chromium.org/628703005/diff/1/content/
> common/gpu/client/command_buffer_proxy_impl.cc
>
>> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
>>
>
>
> https://codereview.chromium.org/628703005/diff/1/content/
> common/gpu/client/command_buffer_proxy_impl.cc#newcode512
>
>> content/common/gpu/client/command_buffer_proxy_impl.cc:512: const
>> std::vector<ui::LatencyInfo>& latency_info) {
>> It doesn't look like you have any limits on the amount of data that can be
>>
> sent
>
>> back and forth over this message. It seems like calling
>> LatencyInfo::Verify on
>> this vector would be a reasonable way of enforcing sanity checks. WDYT?
>>
>
> Thanks for catching that! I have updated the patch to match the
> verification as
> it exists today (gpu_process_host_ui_shim.cc).
>
> Do we mostly care about RenderWidgetHostImpl::CompositorFrameDrawn()
> iterating
> through all those elements with a malicious renderer?
> What about all the allocations and copying we go through? Should we really
> be
> overriding the IPC traits for vector<LatencyInfo> so that it limits the
> size
> there?
>
> Should we also check this earlier in DelegatedFrameHost (and RWHVAndroid)
> when
> we receive the original info from the renderer, since I assume we trust the
> renderer less than the gpu process.
>

I don't think we need to check the size of this for allocation. IPCs are
already limited in size (IPC::Channel::kMaximumMessageSize = 128MB), that's
checked on the receiving side.

TBH, I think Verify on the receiving side only is unfortunate, there could
be extreme, but legitimate cases where LatencyInfos come from many
renderers and get merged, which would lead to us dropping the data silently.


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-10-07 20:20:51]

https://codereview.chromium.org/628703005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/628703005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:519:
swap_buffers_completion_callback_.Run(std::vector<ui::LatencyInfo>());
return? We don't want to run the callback twice, do we?

[no sievers, 2014-10-07 20:22:24]

https://codereview.chromium.org/628703005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/628703005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:519:
swap_buffers_completion_callback_.Run(std::vector<ui::LatencyInfo>());
On 2014/10/07 20:20:51, piman (Very slow to review) wrote:
> return? We don't want to run the callback twice, do we?

Doh! Done.

[Mike West, 2014-10-08 07:29:19]

> > Should we really be overriding the IPC traits for vector<LatencyInfo> so
that
> > it limits the size there?

I don't think you need to do that, calling verify at the entry point seems like
enough. If you'd like to poke at moving verification up (down?) the stack,
however, feel more than free. :)

> > Should we also check this earlier in DelegatedFrameHost (and RWHVAndroid)
> > when
> > we receive the original info from the renderer, since I assume we trust the
> > renderer less than the gpu process.

We'd like not to trust the renderer at all, if we can avoid it. The GPU process
isn't much more trustworthy, really. That's rule #1 of
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....
:)
 
> I don't think we need to check the size of this for allocation. IPCs are
> already limited in size (IPC::Channel::kMaximumMessageSize = 128MB), that's
> checked on the receiving side.

This patch removes a call to Verify (from GpuProcessHostUIShim::OnFrameDrawn);
it seems reasonable to sanity check the data that we're receiving, and not to
remove sanity checks that are already in place.

> TBH, I think Verify on the receiving side only is unfortunate, there could
> be extreme, but legitimate cases where LatencyInfos come from many
> renderers and get merged, which would lead to us dropping the data silently.

In the same message? Currently, it looks like this patch is calling ::Verify on
each message as it comes in; are there legitimate cases where you'd expect a
swap's LatencyInfo vector to exceed the sizes that verify expects?

[piman, 2014-10-08 17:24:50]

On Wed, Oct 8, 2014 at 12:29 AM, <mkwst@chromium.org> wrote:

> > Should we really be overriding the IPC traits for vector<LatencyInfo> so
>>
> that
>
>> > it limits the size there?
>>
>
> I don't think you need to do that, calling verify at the entry point seems
> like
> enough. If you'd like to poke at moving verification up (down?) the stack,
> however, feel more than free. :)
>
>  > Should we also check this earlier in DelegatedFrameHost (and
>> RWHVAndroid)
>> > when
>> > we receive the original info from the renderer, since I assume we trust
>> the
>> > renderer less than the gpu process.
>>
>
> We'd like not to trust the renderer at all, if we can avoid it. The GPU
> process
> isn't much more trustworthy, really. That's rule #1 of
> http://www.chromium.org/Home/chromium-security/education/
> security-tips-for-ipc#TOC-Trust-only-the-browser-process.
> :)
>
>  I don't think we need to check the size of this for allocation. IPCs are
>> already limited in size (IPC::Channel::kMaximumMessageSize = 128MB),
>> that's
>> checked on the receiving side.
>>
>
> This patch removes a call to Verify (from GpuProcessHostUIShim::
> OnFrameDrawn);
> it seems reasonable to sanity check the data that we're receiving, and not
> to
> remove sanity checks that are already in place.
>
>  TBH, I think Verify on the receiving side only is unfortunate, there could
>> be extreme, but legitimate cases where LatencyInfos come from many
>> renderers and get merged, which would lead to us dropping the data
>> silently.
>>
>
> In the same message? Currently, it looks like this patch is calling
> ::Verify on
> each message as it comes in; are there legitimate cases where you'd expect
> a
> swap's LatencyInfo vector to exceed the sizes that verify expects?
>

LatencyInfo vectors are merged together in a bunch of places. See e.g.
DelegatedFrameHost::SwapDelegatedFrame or
CommandBufferProxyImpl::SetLatencyInfo

The 100 limit in Verify is completely arbitrary. If a user has >100 tabs
open and they all swap a frame, it's a legitimate case but it would lose
some the LatencyInfos.


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2014-10-08 18:04:58]

On 2014/10/08 07:29:19, Mike West wrote:

> This patch removes a call to Verify (from GpuProcessHostUIShim::OnFrameDrawn);
> it seems reasonable to sanity check the data that we're receiving, and not to
> remove sanity checks that are already in place.
> 

Yes I readded it now in content/common/gpu/client/command_buffer_proxy_impl.cc.
It comes in on a different channel now, but the check now should be equivalent
to before.

[piman, 2014-10-08 20:24:21]

lgtm

[no sievers, 2014-10-09 18:23:30]

ping @tsepez for IPC review

[Tom Sepez, 2014-10-09 18:31:36]

Owners stamp LGTM for ipc messages.

[no sievers, 2014-10-09 20:23:47]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:25:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628703005/150001

[commit-bot: I haz the power, 2014-10-09 20:40:21]

Committed patchset #6 (id:150001)

[commit-bot: I haz the power, 2014-10-09 20:41:38]

Patchset 6 (id:??) landed as
https://crrev.com/df212b3be96421b8b0bcce4947a50236abc38615
Cr-Commit-Position: refs/heads/master@{#298972}