Avoids releasing pointer grab prematurely

Avoids releasing pointer grab prematurely

BUG=338247
TEST=
1.Launch chrome://settings..ctrl+D,save the Bookmark in Bookmark Bar.
2.Open 3 to 4 new Tabs via ctrl+T,in chrome://settings page Drag the Bookmark from the Bookmark Bar.
3.Hover the Dragged Bookmark over the second New-Tab and Release the Bookmark over the New-Tab.
4.Hover the Dragged Bookmark over the third New-Tab (the tab gets activated) and Release the Bookmark over the New-Tab.
5.Close browser - should not crash.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252201

[varkha, 2014-02-06 07:40:04]

erg@, Could you please take a look? The capture that is acquired in a call to
GrabPointerWithCursor (line 115) was released when a window that had capture was
losing focus (in DesktopWindowTreeHostX11::OnNativeWidgetBlur). Keeping the
ScopedCapturer in a smaller scope and releasing it before calling
GrabPointerWithCursor helps.
The result of that premature release was that the pointer movement and button
release were no longer delivered to the drag input window causing hang and crash
on exit. Thanks!

[Elliot Glaysher, 2014-02-06 18:12:17]

Adding sadrul, who wrote this.

[sadrul, 2014-02-06 19:14:46]

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:113: // below.
I do not understand what this means. Mind explaining with more detail?

The capture is meant to persist during the entire drag. Releasing the capture
early here seems wrong?

[varkha, 2014-02-06 19:22:55]

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:113: // below.
On 2014/02/06 19:14:46, sadrul wrote:
> I do not understand what this means. Mind explaining with more detail?
> 
> The capture is meant to persist during the entire drag. Releasing the capture
> early here seems wrong?

There are three points where we set capture, all three are involved. The first
point is when DesktopWindowTreeHostX11::SetCapture is called. The second is
ScopedCapturer which releases that capture and sets another and the third is
GrabPointerWithCursor which releases that and grabs another one. This one is
supposed to stay through the drag. It gets broken when one tab looses focus (and
another one gains focus). When that happens the 
DesktopWindowTreeHostX11::OnNativeWidgetBlur releases capture and that is the
same capture as the one obtained last (in GrabPointerWithCursor). The result is
that mouse events are no longer reaching the drag input window.

Limiting the scope causes the capture in DesktopWindowTreeHostX11 (pointed by
x11_capture_ member there) to get released which then allows OnNativeWidgetBlur
to avoid releasing the capture.

[sadrul, 2014-02-07 00:08:36]

Considering the subtleties and brittleness of captures, should we have a single
X11CaptureManager that manages all capture/grab related code?

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:113: // below.
On 2014/02/06 19:22:55, varkha wrote:
> On 2014/02/06 19:14:46, sadrul wrote:
> > I do not understand what this means. Mind explaining with more detail?
> > 
> > The capture is meant to persist during the entire drag. Releasing the
capture
> > early here seems wrong?
> 
> There are three points where we set capture, all three are involved. The first
> point is when DesktopWindowTreeHostX11::SetCapture is called.

In the context of a drag, who calls this first SetCapture()? I suppose it's done
by Widget in response to the mouse-press event?

> The second is
> ScopedCapturer which releases that capture and sets another and the third is
> GrabPointerWithCursor which releases that and grabs another one. This one is
> supposed to stay through the drag. It gets broken when one tab looses focus
(and
> another one gains focus). When that happens the 
> DesktopWindowTreeHostX11::OnNativeWidgetBlur releases capture and that is the
> same capture as the one obtained last (in GrabPointerWithCursor).

A DesktopWindowTreeHostX11 is supposed to receive events only directed to its
|xwindow_|. When the grab happens on |grab_input_window_|, do we get a FocusOut
X-event directed to |xwindow_|? I guess if this happens, then X11ScopedCapture
should not reset the grab in such situations.

If we reset the capture (i.e. the one from ScopedCapture) here and things work,
does it make any difference if we just don't hold the capture (i.e. don't use
ScopedCapture) in the first place?

> The result is
> that mouse events are no longer reaching the drag input window.
> 
> Limiting the scope causes the capture in DesktopWindowTreeHostX11 (pointed by
> x11_capture_ member there) to get released which then allows
OnNativeWidgetBlur
> to avoid releasing the capture.

[varkha, 2014-02-07 00:32:35]

On 2014/02/07 00:08:36, sadrul wrote:
> Considering the subtleties and brittleness of captures, should we have a
single
> X11CaptureManager that manages all capture/grab related code?
> 
>
https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
> File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):
> 
>
https://codereview.chromium.org/140663013/diff/1/ui/views/widget/desktop_aura...
> ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:113: // below.
> On 2014/02/06 19:22:55, varkha wrote:
> > On 2014/02/06 19:14:46, sadrul wrote:
> > > I do not understand what this means. Mind explaining with more detail?
> > > 
> > > The capture is meant to persist during the entire drag. Releasing the
> capture
> > > early here seems wrong?
> > 
> > There are three points where we set capture, all three are involved. The
first
> > point is when DesktopWindowTreeHostX11::SetCapture is called.
> 
> In the context of a drag, who calls this first SetCapture()? I suppose it's
done
> by Widget in response to the mouse-press event?
You are correct here. It is called in response to the first click on a bookmark
- even before there is enough drag to start drag-and-drop move loop.

> > The second is
> > ScopedCapturer which releases that capture and sets another and the third is
> > GrabPointerWithCursor which releases that and grabs another one. This one is
> > supposed to stay through the drag. It gets broken when one tab looses focus
> (and
> > another one gains focus). When that happens the 
> > DesktopWindowTreeHostX11::OnNativeWidgetBlur releases capture and that is
the
> > same capture as the one obtained last (in GrabPointerWithCursor).
> 
> A DesktopWindowTreeHostX11 is supposed to receive events only directed to its
> |xwindow_|. When the grab happens on |grab_input_window_|, do we get a
FocusOut
> X-event directed to |xwindow_|? I guess if this happens, then X11ScopedCapture
> should not reset the grab in such situations.
I think DesktopWindowTreeHostX11 may lack awareness of the move loop or the
inner grab on the |grab_input_window_| but the move loop knows about the host
and therefore can cancel the grab there cleanly before doing grab on the
|grab_input_window_|.


> If we reset the capture (i.e. the one from ScopedCapture) here and things
work,
> does it make any difference if we just don't hold the capture (i.e. don't use
> ScopedCapture) in the first place?
It would matter. We need to release the original capture in the
DesktopWindowTreeHostX11 cleanly (call through into
DesktopWindowTreeHostX11::ReleaseCapture()). This is what I am doing by grabbing
and releasing through the scoped capturer (or I could just release I guess since
that initial [second in this scenario] grab is probably not needed for the first
half of the X11WholeScreenMoveLoop::RunMoveLoop where all we do is
CreateDragInputWindow -- but that would not work since ReleaseCapture is
protected).


> > The result is
> > that mouse events are no longer reaching the drag input window.
> > 
> > Limiting the scope causes the capture in DesktopWindowTreeHostX11 (pointed
by
> > x11_capture_ member there) to get released which then allows
> OnNativeWidgetBlur
> > to avoid releasing the capture.

[varkha, 2014-02-19 20:46:42]

sadrul@, could you please take another look? I have added a comment explaining
why this second capture needs to be scoped in a smaller scope and there is a
comment to explain why it needs to be released before a call to
GrabPointerWithCursor. Let me know if you want to phrase it differently. Thanks!

[sadrul, 2014-02-19 21:43:48]

lgtm

https://codereview.chromium.org/140663013/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/140663013/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:98: // first being
when a mouse is first clicked. That first capture needs to be
first *pressed

[varkha, 2014-02-19 21:47:46]

The CQ bit was checked by varkha@chromium.org

[varkha, 2014-02-19 21:47:59]

https://codereview.chromium.org/140663013/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/140663013/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:98: // first being
when a mouse is first clicked. That first capture needs to be
On 2014/02/19 21:43:49, sadrul wrote:
> first *pressed

Done.

[commit-bot: I haz the power, 2014-02-19 22:14:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/140663013/210001

[commit-bot: I haz the power, 2014-02-20 00:44:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/140663013/210001

[commit-bot: I haz the power, 2014-02-20 03:20:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/140663013/210001

[commit-bot: I haz the power, 2014-02-20 08:44:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/140663013/210001

[commit-bot: I haz the power, 2014-02-20 12:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/140663013/210001

[commit-bot: I haz the power, 2014-02-20 12:59:56]

Change committed as 252201