750061 - firefox locks desktop when dragging tab from tabbar

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Landis Reed]

Attachment: snapshot shows cursor remaining in 'grip' appearance after tab fails to detatch from active window

User Agent: Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120429 Firefox/14.0a2
Build ID: 20120429042006

Steps to reproduce:

Drag tab from 'Tabbar' to any area attempting to 'open' tab in 'new window'


Actual results:

Desktop Locks. Cursor remains a 'fist' as if 'gripping' the tab, but tab 'returns' to tab bar and desktop is locked. I can not click on anything, activate my 'autohide' taskbar, click on 'new snapshot' of my screencapture app when i alt+tab it to for ground. 
I have to ctrl+esc , tab, del (KILL) firefox and then cursor returns to normal and desktop functionality returns (openSuSE 12.1 with KDE 4.7.2 and kernel 3.1.10-1.9-desktop)


Expected results:

tab opens in 'new window'

Comment 1

[Landis Reed]

I can reproduce this over and over and is Not url dependent.

Comment 2

[Landis Reed]

started Aurora in 'basic' profile with No 'add-ons'. Same issue.

Comment 3

[Landis Reed]

just fyi, 'drag and drop' of url to bookmark folder works fine. as does dragging bookmark from folder to another location. Only drag tab 'out of' tabbar.

Comment 4

[Landis Reed]

Drag and Drop Tab from tabbar to bookmark folder works. Slow, but it works. 
Clarify, Drag 'Tab' and drop in BM folder, Not url 'favicon' from 'location bar', but Tab.

Comment 5

[Landis Reed]

*NOTE - attached image, 'gripped' cursor is in center of page header, fyi.

Comment 6

[Alice0775 White]

Step To Reproduce:
1. Un-check "Tabs on Top" in order to easily reproduce the problem
2. Open many tabs(Ex. 3 Tabs)
3. Mouse down on a tab , move mouse downward and release mouse (you should perform these sequence very quickly)

Actual results:
  Mouse cursor remain grab shape.
  UI does not response with mouse


Expected results:
  Mouse cursor should return auto
  UI should response with mouse


Regression window(cached m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/b077059c575a
Mozilla/5.0 (X11; Linux i686; rv:13.0a1) Gecko/20120207 Firefox/13.0a1 ID:20120207031136
Fails:
http://hg.mozilla.org/mozilla-central/rev/b45785802731
Mozilla/5.0 (X11; Linux i686; rv:13.0a1) Gecko/20120207 Firefox/13.0a1 ID:20120207174850
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b077059c575a&tochange=b45785802731

Last good: ca19aff687a1
First bad: 050334f9128c
Triggered by:
050334f9128c	Karl Tomlinson — b=500081 use a timestamp when grabbing the pointer and generate timestamps for drags in the same way r=roc

Comment 7

[Karl Tomlinson (:karlt)]

On trunk:

I can reproduce this only with tabs.onTop.

Speed is not important.

What seems to be important is that the distance dragged across the browser chrome is small.
Pressing the mouse button near the bottom of the tab and dragging directly down is enough.

The drag (and its pointer grab - lock) does not begin until the pointer is moved back over the browser chrome.

The drag session receives drag-failed and drag-end immediately, but the pointer remains grabbed.

The keyboard is not grabbed.  Using a keyboard to open another popup window will cause a new pointer grab and reset the cursor state.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> On trunk:
> 
> I can reproduce this only with tabs.onTop.

I mean only with tabs.onTop disabled.

Comment 9

[Karl Tomlinson (:karlt)]

The regression is due to the timestamp used to start the drag changing from the time of the last button press to the time of the mouse motion, after the button release, that started the drag.

The last button release time used in code added in bug 307184 to end the drag is older than the mouse motion timestamp, and so the pointer ungrab fails.  GTK still thinks it has ended the drag and so things are in a bad state.

The code added in bug 307184 was intended to handle the situation where the button release event is in the event queue at the time the drag started.  It still functions as intended in that situation.

The regression is that the code added in bug 307184 no longer happens to save us from bug 648477, which is when the drag begins after the button release has been processed.

Comment 10

[Karl Tomlinson (:karlt)]

The code from bug 307184 was to work around https://bugzilla.gnome.org/show_bug.cgi?id=347277 which was fixed for GTK+ 2.10.1.
We don't support systems older than 2.12, so we can assume we have the fix.

However, the mHiddenWidget that nsDragService uses as the drag source is not in the same window group as the window where the mouse button was pressed (which is the same window that receives the matching release).

Comment 11

[Karl Tomlinson (:karlt)]

I wondered whether we could remove our window group code, putting all windows in the default group.  Window groups affect grabs so these are my notes on grabs.

There are two types of grabs that Gecko makes use of:
gdk_pointer_grab and gtk_grab_add.

gdk_pointer_grab behaves differently depending on the owner_events parameter.

  If owner_events is false this redirects mouse events to the specified
  (possibly child) GdkWindow only.  This changes behavior only at the X server
  and so doesn't affect events that have already been sent.

  If owner_events is true mouse events from only outside the (toplevel) window
  group are redirected to the specified GdkWindow.  Events that would normally
  be delivered to the window group continue to be delivered as normal.  This
  is implemented partially on the X server, but events sent to the same client
  but different window group are redirected by GTK, so this part is effective
  for events that have already been sent but are still in the queue.

gtk_grab_add means that any event that would be delivered to the same
(toplevel) window group is redirected to the specified widget.  The window in
the event is not changed and so motion_notify_event_cb etc in nsWindow.cpp
redirects it back to the original window anyway.

  gtk_grab_add is implemented in GTK and so affects events already queued.

  gtk_grab_add also influences keyboard events.  It doesn't change the focus
  widget (nor toplevel window) but does redirect keyboard events to the
  specified widget.  key_press_event_cb etc in nsWindow.cpp redirects the
  event back to the focus window.

  For Gecko, apparently the only difference that our gtk_grab_add makes is
  that it keeps events from being delivered to GtkWidgets of plugins.

  gtk_dialog_run uses gtk_grab_add to make the window modal wrt those windows
  in the same group.

If we were to remove the window group code:

1. Bug 741283 would extend to all windows, not just the one that opened the menu.

   That would be a bit sad, but not major.

2. Modal windows would be app-modal instead of window-modal.

   Currently Gecko modal windows are implemented using nested event loops so
   they should be app-modal to function properly.

   However, window-modal dialogs are much more pleasant than app modal dialogs.
   Despite not always working properly, it would seem quite a regression to
   switch to app modal dialogs.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Put the hidden drag widget in the window group of the source node

What I suggest we do is put mHiddenWidget in the same window group as the window where the mouse button was pressed, so that the code added for bug 307184 can be removed.  This will also help with bug 751429.

There is then no button release event that gets dispatched with the wrong timestamp.

This patch does not work around bug 648477, but leaves us in a much better (and un"lock"ed) state.  nsDragService will still start a drag session when asked, but a click will end the drag.  Bug 648477 means that the drag that the user intended to start, starts late.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 620582 [details] [diff] [review]
Put the hidden drag widget in the window group of the source node

I'll explain a couple of things:

>-    mHiddenWidget = gtk_invisible_new();
>+    mHiddenWidget = gtk_window_new(GTK_WINDOW_POPUP);

GtkInvisibles don't have a window group.  This GtkWindow is realized but never shown so it is still invisible.

>-    if (gtk_grab_get_current() == data->mWidget) {
>+    if (gtk_widget_has_grab(data->mWidget)) {

This check isn't exactly the same, but close enough and easier than finding the current grab in the widget's window group.
_has_grab() doesn't on its own imply *the* active grab, but the gtk drag code will immediately start to end the drag if this widget is no longer *the* grab, at which point it will remove its grab.

Comment 14

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd39845edf

Comment 15

[Karl Tomlinson (:karlt)]

We could think about applying this fix to mozilla14 aurora.
It feels too late to consider such a change for mozilla13 beta.

AFAICT the problem exists only we tabs.onTop is set to false or the navigation bar is hidden.

The workaround to escape without killing Firefox is to use the keyboard to open a menu.  e.g Alt+F, but this is not obvious.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ecd39845edf

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for 13 and 14 since it's a regression in 13 as well - can we get a risk assessment here? Why does comment 15 suggest it feels too late to fix this on beta?  We've still got 3 more weeks of beta testing and if this got in today it could go in tomorrow's beta 3 build.

Comment 18

[Karl Tomlinson (:karlt)]

There is a moderate risk with this fix.  Drags involve coordination across a number of actions and depend on GTK behaviour.  If any part of the effort doesn't behave as expected, then things can fall over.

The other option to consider is backing out bug 500081, bug 725047, bug 724966, bug 724967, but there is also risk in doing that if something else is now depending on the bugs that were fixed.

I'd recommend applying the fix to alpha, but with this bug affecting only tabs.onTop or missing navigation bar, I don't know whether it's worth adding risk to beta/13.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 620582 [details] [diff] [review]
Put the hidden drag widget in the window group of the source node

[Approval Request Comment]
Regression caused by (bug #): 500081
User impact if declined: comment 15, comment 18
Testing completed (on m-c, etc.): on m-c, comment 16
Risk to taking this patch (and alternatives if risky): see comment 18
String changes made by this patch: none

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #18)
> with this bug affecting only tabs.onTop

Sorry, I mean non-default tabs.onTop set to false.

> or missing navigation bar

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 620582 [details] [diff] [review]
Put the hidden drag widget in the window group of the source node

[Triage Comment]
Given the risk assessment and non-default STR for Firefox, approving for Aurora 14 and declining for Beta 13. CC'ing Callek in case he wants to take this fix for SM though.

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: aurora patch

On aurora, we need to add a gtk2compat.h include and make nsWindow::GetMozContainerWidget() public, as these are not on Aurora yet.  I've picked these changes out of 
http://hg.mozilla.org/mozilla-central/rev/ce526785dc49 and
http://hg.mozilla.org/mozilla-central/rev/3c6358d51fa5 where they landed for bug 497498.

[Approval Request Comment]
Re-requesting approval.
The additional changes are small and don't change the risk assessment.

Comment 23

[Landis Reed]

10.05.12 - Still Happening, but More Frequently.. Getting very annoying. 

Landis.

Comment 25

[Karl Tomlinson (:karlt)]

Landis, the fix hasn't landed for Aurora yet, but please let us know if this is still happening with Nightly.

Comment 26

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35c8a9151452

Comment 27

[Landis Reed]

Just want to say Thank You...
The last two days, while the tab still periodically gets 'stuck' to the mouse pointer, FF (Aurora) Does NOT hang.. 

Thank you Everyone who helped.
Landis.

Comment 28

[Landis Reed]

(In reply to Karl Tomlinson (:karlt) from comment #18)
> There is a moderate risk with this fix.  Drags involve coordination across a
> number of actions and depend on GTK behaviour.  If any part of the effort
> doesn't behave as expected, then things can fall over.
> 
> The other option to consider is backing out bug 500081, bug 725047, bug
> 724966, bug 724967, but there is also risk in doing that if something else
> is now depending on the bugs that were fixed.
> 
> I'd recommend applying the fix to alpha, but with this bug affecting only
> tabs.onTop or missing navigation bar, I don't know whether it's worth adding
> risk to beta/13.

Karl, just to let you know. I've Never run ff with 'tabs.ontop' and always have nav bar visible. My experience with this issue has always been with tabs on the bottom and 'location bar' visible.. Just letting you know so you make fully informed decisions.

Thanks for all you do.. I try and follow, but you guys (gals) lose me when your really get going.. : )
Landis.

Comment 29

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Verified on Ubuntu 12.04 in F14 beta7. 
Detaching tabs works as expected - both with tabs on top enabled and disabled. No hang/desktop lock.