GTK: add more syncronization to GtkNativeViewIdManager to avoid a crash.

GTK: add more syncronization to GtkNativeViewIdManager to avoid a crash.

BUG=46197
TEST=manual (see bug)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50886

[Evan Stade, 2010-06-25 00:23:10]

Ignore the last email, that was sent for the wrong cl

===============================

This seems to fix the problem (and I don't think it's susceptible to deadlock).

tangentially: I noticed a bunch of functions that are sort of overlapping (one
each written by Dan, one Adam):

x11_util::GetWindowParent()/x11_util::GetParentWindow()

x11_util::GetHighestAncestorWindow()/GetTopLevelWindow in
ResourceMessageFilterGtk

both of the GetTopLevelWindow ones are implemented by calling the
GetParentWindow ones in a loop. Is there some reason not to just use
XQueryWindow once and trust the root window it gives us?

[agl, 2010-06-25 01:11:45]

LGTM

http://codereview.chromium.org/2860024/diff/2001/3003
File gfx/gtk_native_view_id_manager.h (right):

http://codereview.chromium.org/2860024/diff/2001/3003#newcode92
gfx/gtk_native_view_id_manager.h:92: // id. So this records the current mapping.
I guess I'm pretty much resigned to the fact that nobody likes my 'indent the
protected members' style.

[Evan Stade, 2010-06-25 01:36:06]

http://codereview.chromium.org/2860024/diff/2001/3003
File gfx/gtk_native_view_id_manager.h (right):

http://codereview.chromium.org/2860024/diff/2001/3003#newcode92
gfx/gtk_native_view_id_manager.h:92: // id. So this records the current mapping.
On 2010/06/25 01:11:45, agl wrote:
> I guess I'm pretty much resigned to the fact that nobody likes my 'indent the
> protected members' style.

oh, I didn't even realize that was the intent. I'll change it back if you want.

[Evan Martin, 2010-06-25 02:22:13]

I think the whole reason we have a background X11 thread is so we don't block
the UI thread on its actions.  Doing this is perhaps equivalent to just getting
rid of the background X11 thread and doing its calls on the UI thread (which I'm
also not especially opposed to).

[Evan Stade, 2010-06-25 04:00:44]

well, it's not quite the same. Both sides of the code that acquire the lock
execute quickly and can't deadlock each other, whereas apparently there's some
chance for deadlock in the general "renderer blocking on ui thread" case
(although I don't know what causes it---I thought the browser was never supposed
to block on the renderer anyway).

[Evan Martin, 2010-06-25 05:15:30]

On 2010/06/25 04:00:44, Evan Stade wrote:
> well, it's not quite the same. Both sides of the code that acquire the lock
> execute quickly and can't deadlock each other, whereas apparently there's some
> chance for deadlock in the general "renderer blocking on ui thread" case
> (although I don't know what causes it---I thought the browser was never
supposed
> to block on the renderer anyway).

That's right -- the renderer already blocks on the browser.  It's the other way
around that causes deadlocks.  :)

[agl, 2010-06-25 05:47:10]

On Thu, Jun 24, 2010 at 9:36 PM,  <estade@chromium.org> wrote:
> oh, I didn't even realize that was the intent. I'll change it back if
> you want.

Nah. That's the problem: nobody gets it :)


AGL

[agl, 2010-06-25 05:49:07]

On Thu, Jun 24, 2010 at 10:22 PM,  <evan@chromium.org> wrote:
> I think the whole reason we have a background X11 thread is so we don't
> block
> the UI thread on its actions.  Doing this is perhaps equivalent to just
> getting
> rid of the background X11 thread and doing its calls on the UI thread (which
> I'm
> also not especially opposed to).

The issue is the UI thread will go out to lunch for 100s of
milliseconds at a time. It's still pumping UI actions, but it doesn't
pump its Chrome message loop, so blocking the renderer on that is bad.
(Same reason that fontconfig requests are handled in the helper
process, not the UI thread.) This patch doesn't suffer from that.


AGL

[Evan Martin, 2010-06-25 06:49:18]

On Thu, Jun 24, 2010 at 10:48 PM, Adam Langley <agl@chromium.org> wrote:
> On Thu, Jun 24, 2010 at 10:22 PM,  <evan@chromium.org> wrote:
>> I think the whole reason we have a background X11 thread is so we don't
>> block
>> the UI thread on its actions.  Doing this is perhaps equivalent to just
>> getting
>> rid of the background X11 thread and doing its calls on the UI thread (which
>> I'm
>> also not especially opposed to).
>
> The issue is the UI thread will go out to lunch for 100s of
> milliseconds at a time.

How?  You mean in response to these requests?  Or for other reasons?

> It's still pumping UI actions, but it doesn't
> pump its Chrome message loop, so blocking the renderer on that is bad.

It's the same pump, so I don't think you can get UI actions without
Chrome messages.  Or equivalently they're both blocked at the same
times.  If the UI is blocked then the rest of the app might as well be
blocked, no?

[agl, 2010-06-25 18:58:42]

On Fri, Jun 25, 2010 at 2:48 AM, Evan Martin <evan@chromium.org> wrote:
> It's the same pump, so I don't think you can get UI actions without
> Chrome messages.  Or equivalently they're both blocked at the same
> times.  If the UI is blocked then the rest of the app might as well be
> blocked, no?

It certainly didn't appear so at the time. I suspect that X messages
were handled first and, when animating etc, the Chrome MessageLoop
would stall.


AGL