Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include <utility>
 
@@ skipping 431 matching lines @@
       using_browser_compositor_(CompositorImpl::IsInitialized()),
       synchronous_compositor_client_(nullptr),
       frame_evictor_(new DelegatedFrameEvictor(this)),
       locks_on_frame_count_(0),
       observing_root_window_(false),
       weak_ptr_factory_(this) {
   // Set the layer which will hold the content layer for this view. The content
   // layer is managed by the DelegatedFrameHost.
   view_.SetLayer(cc::Layer::Create());
   if (using_browser_compositor_) {
+    cc::FrameSinkId frame_sink_id = cc::FrameSinkId(
+        base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),

[boliu, 2017/01/26 00:14:39]

these numbers are ok as the "client id" and "sink id"? maybe should document
what these IDs are somewhere and why this is ok?

[xlai (Olivia), 2017/01/26 15:56:11]

Yes, clientId+SinkId is the frame sink id. Again this is copied from the same
piece of code as non-Mac non-Android platform so as ensure platform consistency.
But here I drop the tertiary operator because is_guest_view_hack_ is never true
in Android.

[Fady Samuel, 2017/01/26 16:06:36]

The FrameSinkId uniquely identifies a CompositorFrameSink (and one day a
ContentFrameSink). FrameSinkId is the persistent (across display compositor
restarts) component of a SurfaceId. A FrameSinkId allows the window server or
browser to know which client submitted a CompositorFrame in response to
OnSurfaceCreated message from the display compositor.

In the Mus window server, each FrameSinkId uniquely identifies a Window (an
embeddable client). In the browser, that is also the case, more or less.
DelegatedFrameHostAndroid is the content area "window" (embeddable client).

In Chrome today, we use the process ID and routing ID combination to uniquely
identify a client. Thus It makes sense for FrameSinkIds to hold the process ID /
routing ID pair. We already ran this by security (tsepez@ in particular).

The interesting this about this pair is FrameSinkId consists of a component
picked by the privileged process (client ID) and a component that can be picked
by an unprivileged process (the sink ID). This enables the client to allocate a
unique ID for embeddable children as is the case with offscreen canvas.

[boliu, 2017/01/26 19:24:45]

I was mostly asking for correctness.

So for renderers, FrameSinkId uses process/routing id. But right now the browser
compositor still uses
ui::ContextProviderFactory::GetInstance()->AllocateFrameSinkId, and it's not
clear to me at this high level that a renderer won't collide with the browser,
since their IDs come from completely independent sources?

Or is there still something I'm misunderstanding?

[Fady Samuel, 2017/01/26 20:15:22]

Good catch on Android, we don't AllocateFrameSinkIds in the browser correctly:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/context_pr...

In GpuProcessTransportFactory, we leave the client ID component 0 (renderers
have a RenderProcessHost ID > 0) and so there are no collisions.

https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...

Olivia, could you please fix this? Thanks!

[xlai (Olivia), 2017/01/26 20:50:21]

I just made a fix to ContextProviderFactory in Android so that the generation of
FrameSinkId will be exactly same as in non-Android platform. Basically, browser
process will have the 0 as its client ID; renderer processes get their client ID
starting from 1 so they will never collide.

+        base::checked_cast<uint32_t>(host_->GetRoutingID()));
     delegated_frame_host_.reset(new ui::DelegatedFrameHostAndroid(
-        &view_, cached_background_color_, this));
+        &view_, cached_background_color_, this, frame_sink_id));
   }
 
   host_->SetView(this);
   SetContentViewCore(content_view_core);
 
   CreateOverscrollControllerIfPossible();
 
   if (GetTextInputManager())
     GetTextInputManager()->AddObserver(this);
 }
@@ skipping 1603 matching lines @@
   ui::WindowAndroidCompositor* compositor = window_android->GetCompositor();
   if (!compositor)
     return;
 
   overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(
       overscroll_refresh_handler, compositor,
       ui::GetScaleFactorForNativeView(GetNativeView()));
 }
 
 }  // namespace content