GPU compositing surface handle is no longer sent to renderer process.

GPU compositing surface handle is no longer sent to renderer process.

Instead it is stored in a map in RenderWidgetHelper indexed by RenderWidgetHost route ID. This allows the UI thread to maintain the mapping as windows are created and destroyed and the IO thread to lookup the mapping in order to create GL contexts that render to the windows.

This avoids a race where JavaScript would open a popup window and immediately try to use an accelerated canvas to render to it (2D canvas or WebGL canvas). <-- This is no longer true of this patch. There was a potential deadlock.

WebGL canvas used to work in this case only because it would fall back to using ReadPixels.

This goes some way to fixing the bug referenced below but does not fix it completely.BUG=80703
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90617

[apatrick_chromium, 2011-06-13 21:15:59]

backer: can you review this.

jam: can you take a look at gpu_message_filter.cc and check this doesn't create
a potential deadlock. In summary the browser's IO thread needs to be able to get
the window handle for a view.

There is now a lock protected map from view id to handle but if the renderer
process opens a popup and immediately trys to render to it using an accelerated
canvas, the IPC requesting a command buffer can arrive on the browser's IO
thread before its UI thread has created the window and added it to the map. In
this case the IO thread defers replying to the IPC until the UI thread has been
"flushed" and the task to create the window completed.

I tested it by adding an artificial call to sleep to slow down the creation of
the window and trigger the race.

I also need owners LGTM.

[jam, 2011-06-13 22:09:47]

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
File content/browser/renderer_host/gpu_message_filter.cc (right):

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
content/browser/renderer_host/gpu_message_filter.cc:214: // and if not, flush
the tasks in the UI thread before retrying and replying
if the IO thread can potentially wait on the UI thread, deadlock will happen. 
in this case, the DCHECKs in BrowserMessageFilter don't see this because a
method factory is being used, but it's still potential deadlock.

[apatrick_chromium, 2011-06-13 22:13:02]

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
File content/browser/renderer_host/gpu_message_filter.cc (right):

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
content/browser/renderer_host/gpu_message_filter.cc:214: // and if not, flush
the tasks in the UI thread before retrying and replying
On 2011/06/13 22:09:47, John Abd-El-Malek wrote:
> if the IO thread can potentially wait on the UI thread, deadlock will happen. 
> in this case, the DCHECKs in BrowserMessageFilter don't see this because a
> method factory is being used, but it's still potential deadlock.

Is it waiting though? The IO thread will still respond to IPCs and tasks posted
from the UI thread. It's just deferring the reply on one IPC channel. I'll
believe you if it is a potential deadlock, I just need to know why so I can fix
it :)

[jam, 2011-06-13 23:45:12]

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
File content/browser/renderer_host/gpu_message_filter.cc (right):

http://codereview.chromium.org/7136001/diff/9018/content/browser/renderer_hos...
content/browser/renderer_host/gpu_message_filter.cc:214: // and if not, flush
the tasks in the UI thread before retrying and replying
On 2011/06/13 22:13:02, apatrick_chromium wrote:
> On 2011/06/13 22:09:47, John Abd-El-Malek wrote:
> > if the IO thread can potentially wait on the UI thread, deadlock will
happen. 
> > in this case, the DCHECKs in BrowserMessageFilter don't see this because a
> > method factory is being used, but it's still potential deadlock.
> 
> Is it waiting though? The IO thread will still respond to IPCs and tasks
posted
> from the UI thread. It's just deferring the reply on one IPC channel. I'll
> believe you if it is a potential deadlock, I just need to know why so I can
fix
> it :)

np, I know it's not straightforward :)

The issue is not the IO thread, it's the renderer process.  If the renderer
process ever has to wait on a sync message that could potentially need to run on
the UI thread, that causes the deadlock (on Windows).  Imagine the main thread
is painting, and as part of painting child windows, Windows internally waits on
a child window in the plugin process.  At the same time, the plugin process is
making an npruntime call to the renderer.  But the renderer is blocked on the
browser UI thread.

[jonathan.backer, 2011-06-14 14:51:06]

I took a look. Unfortunately, I don't have much to contribute here, other than
the obvious:

- create windows on demand so that a valid handle is always passed; make them
small and offscreen and not owned by a render widget initially; transfer
ownership and characteristics (e.g. parenting and geometry as the init process
progresses)

- currently pop-ups can do some processing before receiving their compositing
surface handle; block until it receives that handle --- I'm sure that this will
cause other timing issues

- create a context with an pbuffer surface and swap it for a  window surface
once a window handle becomes available; IIRC, apatrick was working on just this
type of framework, but there are driver problems

[apatrick_chromium, 2011-06-20 19:42:05]

Fixed potential deadlock. PTAL as before.

The IO thread no longer "flushes" the UI thread and the race is still there. I
will fix by allowing the canvas contexts to be "reparented" to the view context
when the window is actually available. That avoids the deadlock. That is partly
implemented by this:

http://codereview.chromium.org/7205012/

The plumbing to defer reparenting of canvas is a work in progress.

I still want to check this in because I think the browser should not trust the
renderer to give it a valid window handle to draw to. It could be anything.

[jam, 2011-06-21 21:49:03]

lgtm with nits

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/gpu_message_filter.h (right):

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/gpu_message_filter.h:56: RenderWidgetHelper*
render_widget_helper_;
this should be a scoped_refptr

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/gpu_message_filter.h:58:
ScopedRunnableMethodFactory<GpuMessageFilter> method_factory_;
nit: doesnt look like this is used anymore

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_helper.h (right):

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_helper.h:17: #include
"content/browser/browser_thread.h"
nit: doesn't look like this is needed

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:147: void
set_view(RenderWidgetHostView* view);
nit: if you're doing work in this function, then it can't use unix_hacker naming
anymore

[apatrick_chromium, 2011-06-23 01:24:28]

On 2011/06/21 21:49:03, John Abd-El-Malek wrote:
> lgtm with nits
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> File content/browser/renderer_host/gpu_message_filter.h (right):
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> content/browser/renderer_host/gpu_message_filter.h:56: RenderWidgetHelper*
> render_widget_helper_;
> this should be a scoped_refptr
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> content/browser/renderer_host/gpu_message_filter.h:58:
> ScopedRunnableMethodFactory<GpuMessageFilter> method_factory_;
> nit: doesnt look like this is used anymore
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_helper.h (right):
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_helper.h:17: #include
> "content/browser/browser_thread.h"
> nit: doesn't look like this is needed
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host.h (right):
> 
>
http://codereview.chromium.org/7136001/diff/15001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host.h:147: void
> set_view(RenderWidgetHostView* view);
> nit: if you're doing work in this function, then it can't use unix_hacker
naming
> anymore

Done