539862 - dragging JAR file onto Themes pane in add-ons manager doesn't work

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

STR:
1. Download a theme file to the desktop
2. Open Add-ons Manager
3. Switch to Themes pane
4. Drag theme file from desktop to Themes pane

expected: the theme would be installed
actual: drag doesn't work, error printed to console

The error is:

(firefox-bin:15553): Gtk-CRITICAL **: gtk_drag_get_data: assertion `GTK_IS_WIDGET (widget)' failed

Not sure if this is a regression, I didn't test in 3.5.x, but it is a litmus test.

Comment 1

[Neil Deakin]

Attachment: reorder some code

I only tested this on trunk.

Several related issues caused this, due to the order that various initialization steps occur when dragging from an external source over a Mozilla window. Both TargetSetLastContext and UpdateDragStatus need to be called before the dragenter event fire, otherwise the current window and the current drag action aren't set properly.

One issue unfixed is occasionally a dragleave timer is still active during a dragenter, so a dragleave timer fires incorrectly, causing us to think the drag session has ended when it hasn't. This can happen when retrieving the drag data if the timer event is processed during the event loop in nsDragService::GetTargetDragData. I'm not sure of the way to fix this though.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 423041 [details] [diff] [review]
reorder some code

(In reply to comment #1)
> One issue unfixed is occasionally a dragleave timer is still active during a
> dragenter, so a dragleave timer fires incorrectly, causing us to think the drag
> session has ended when it hasn't. This can happen when retrieving the drag data
> if the timer event is processed during the event loop in
> nsDragService::GetTargetDragData. I'm not sure of the way to fix this though.

This is not directly related to your patch, but, in OnDragDropEvent,

>+    CheckNeedDragLeaveEnter(innerMostWidget, dragService, aDragContext, retx, rety);
> 
>     // clear any drag leave timer that might be pending so that it
>     // doesn't get processed when we actually go out to get data.
>     if (mDragLeaveTimer) {
>         mDragLeaveTimer->Cancel();
>         mDragLeaveTimer = nsnull;
>     }
> 
>-    // set the last window to this
>-    mLastDragMotionWindow = innerMostWidget;
>-

can mDragLeaveTimer be canceled before CheckNeedDragLeaveEnter is called?

Should mLastDragMotionWindow::mDragLeaveTimer be similarly canceled here and in OnDragMotionEvent?  mLastDragMotionWindow will be incorrect when the timer fires and OnDragLeave would have already been called (on the correct window).

Comment 3

[Karl Tomlinson (:karlt)]

If it helps at all, I don't think we actually need a time delay in mDragLeaveTimer.  The drag-leave and drag-drop signals are sent during the same event, so scheduling a high-priority (GLib) event with no delay might make the event run before other events are processed.  The event would still need to be canceled from OnDragDropEvent.

Comment 4

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/4ab5e6695dc8

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> One issue unfixed is occasionally a dragleave timer is still active during a
> dragenter, so a dragleave timer fires incorrectly, causing us to think the drag
> session has ended when it hasn't. This can happen when retrieving the drag data
> if the timer event is processed during the event loop in
> nsDragService::GetTargetDragData.

The patches in bug 497498 deal with this.