gpu: Take surface handle into account when importing buffers.

gpu/ipc/service/gpu_command_buffer_stub.cc

Index: gpu/ipc/service/gpu_command_buffer_stub.cc
diff --git a/gpu/ipc/service/gpu_command_buffer_stub.cc b/gpu/ipc/service/gpu_command_buffer_stub.cc
index a3fec2cc52e6b9f7096119869b8035c3bb6e02c5..3aa4b78049db99e554b7a668f7ce2e7e8f517511 100644
--- a/gpu/ipc/service/gpu_command_buffer_stub.cc
+++ b/gpu/ipc/service/gpu_command_buffer_stub.cc
@@ -1005,7 +1005,7 @@ void GpuCommandBufferStub::OnCreateImage(
   }
 
   scoped_refptr<gl::GLImage> image = channel()->CreateImageForGpuMemoryBuffer(
-      handle, size, format, internalformat);
+      handle, size, format, internalformat, surface_handle_);

[piman, 2016/07/14 18:32:44]

surface_handle_ will be null for offscreen contexts (everything except the
Display context in the browser - which currently is also the browser compositor
context, but may not stay that way forever, because of the mus story and the
intent to share contexts more broadly).

At the very least, is this handled at the lower level, security-wise?

AFAICT it goes down to DrmThread::CreateBufferFromFds which then calls
device_manager_->GetDrmDevice(widget), expecting the result to be !null - is it
going to be true?

Moving forward, would it make sense to pass the surface_handle explicitly here?

[reveman, 2016/07/14 20:00:59]

hm, I'm not sure I understand exactly what this context sharing would mean. Will
we potentially use an offscreen context for onscreen compositing? Or do we just
need offscreen contexts that are not necessarily associated with the primary drm
device?
I updated to DrmDeviceManager::GetDrmDevice to not crash if passed an invalid
widget in latest patch as that seems consistent with
DrmDeviceManager::RemoveDrmDevice. However, I'd be surprised if a client managed
to create a context for an invalid widget and that did not causing a failure or
crash somewhere else.
GetDrmDevice will always return !null and handles null input by returning the
primary device. I didn't handle an invalid widget as input in a safe way but
that's been addressed in latest patch as mentioned above.
Explicitly as part of the CHROMIUM_image extension?

[piman, 2016/07/14 20:39:26]

The latter. Today the browser compositor (ui::Compositor) doesn't own the
GLRenderer, it sends a CompositorFrame to the Display - which then itself uses a
GLRenderer to produce the final frame.
They just happen to use the same context (which is a view context, which has a
!null surface_handle_), but they don't have to.
Ok, I see. So if the surface_handle_ is null (for an offscreen context), we'll
revert to the primary device?
Possibly, or maybe implicitly as an internal detail of the implementation?

I guess I'm not sure I fully grok the constraint here. Is it that this surface
handle must match the handle the image will be swapped/overlayed to?

[reveman, 2016/07/14 21:23:40]

Ok, creating an image for one context (drm device) and then trying to schedule
it as an overlay with another context/device is not really supported. Maybe the
solution is to just make sure the normal backbuffer image is always created for
the Display context that will schedule it as an overlay and overlay scheduling
of buffers associated with different drm devices fails gracefully with fallback
to normal compositing.
Correct.
Yes. Current code does this implicitly but it of course relies on having a
surface handle to find the drm device to use and that might need to be improved
for us to be able to create images using an offscreen context without a surface
handle that can be swapped/overlayed using a context that is not associated with
the primary drm device.

Can we have contexts in the same share group that use different drm devices? If
not then maybe that's a good way to find out what the appropriate drm device for
imports are when creating images using an offscreen context.

   if (!image.get())
     return;