After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 751401 - crash on DnD
crash on DnD
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
3.17.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
: 752282 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-06-23 18:01 UTC by Ben
Modified: 2015-07-13 13:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace (2.63 KB, text/plain)
2015-06-23 18:01 UTC, Ben
  Details
Better backtrace (45.23 KB, text/plain)
2015-06-23 23:36 UTC, Michael Catanzaro
  Details
gtkdnd: Account for setting a same icon helper (940 bytes, patch)
2015-07-06 17:36 UTC, Carlos Garnacho
reviewed Details | Review
gtkdnd: Account for setting a same icon helper (972 bytes, patch)
2015-07-06 19:22 UTC, Carlos Garnacho
accepted-commit_now Details | Review

Description Ben 2015-06-23 18:01:58 UTC
Created attachment 305945 [details]
backtrace

Open a documentation page, such as GtkListBoxRow, and drag the image a bit.

devhelp doesn't have this problem
Comment 1 Christian Hergert 2015-06-23 19:39:21 UTC
(In reply to Ben from comment #0)
> devhelp doesn't have this problem

Try running devhelp from `jhbuild shell` and you'll find it too crashes on your test case.
Comment 2 Michael Catanzaro 2015-06-23 23:36:45 UTC
Created attachment 305966 [details]
Better backtrace

It affects WebKitGTK+ 2.4.9 as well as my build of WebKit master when running in my jhbuild shell, so it seems like it should not be a regression in WebKit. But WebKit is calling gtk_drag_begin() with a null GdkEvent, which is surely wrong.

The bug occurs in Fedora rawhide as well (without jhbuild), but not in Fedora 22.

Note: it took me half an hour and six attempts to get this backtrace :p
Comment 3 Michael Catanzaro 2015-06-24 00:08:57 UTC
In WebKit::DragAndDropHandler::startDrag() we assume that gtk_get_current_event() returns nonnull, which was previously a valid assumption, but now it is returning null.

I think we should return early there if gtk_get_current_event() returns null, since otherwise the untrusted web process could force the UI process to crash with fake startDrag messages. I will file a WebKit bug for that. That would fix the crash, but I guess drag-and-drop would still be broken. Let's use this bug to figure out why gtk_get_current_event() is returning null. I bisected it:

3ae953092a4089c6073d1df1546e50daf7099e4c is the first bad commit
commit 3ae953092a4089c6073d1df1546e50daf7099e4c
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Sat May 16 23:55:09 2015 -0400

    Don't force an icon window
    
    We were inadvertently forcing the use of an icon window
    in all cases. This patch makes it so that we once again
    use a combined cursor when possible.
Comment 4 Michael Catanzaro 2015-06-24 00:41:24 UTC
I submitted a WebKit patch to fix the crash, but drag-and-drop is still broken.
Comment 5 Carlos Garnacho 2015-07-06 17:25:56 UTC
(In reply to Michael Catanzaro from comment #3)
> 3ae953092a4089c6073d1df1546e50daf7099e4c is the first bad commit
> commit 3ae953092a4089c6073d1df1546e50daf7099e4c
> Author: Matthias Clasen <mclasen@redhat.com>
> Date:   Sat May 16 23:55:09 2015 -0400

gtk_get_current_event() returns NULL both with and without this patch, you can only expect it to be non-NULL if called within code triggered by gtk_main_do_event(). And gtk_drag_begin() should cope just fine with NULL events, mostly needed for proper timestamps.

That said, ephy indeed doesn't crash with this patch reverted, looks like a real refcounting issue.
Comment 6 Carlos Garnacho 2015-07-06 17:36:15 UTC
Created attachment 306943 [details] [review]
gtkdnd: Account for setting a same icon helper

Just create the new ref before dropping the "old" one, possibly destroying
the object.
Comment 7 Matthias Clasen 2015-07-06 18:08:47 UTC
Review of attachment 306943 [details] [review]:

Might be a nice place for the new g_set_object
Comment 8 Carlos Garnacho 2015-07-06 19:22:12 UTC
Created attachment 306955 [details] [review]
gtkdnd: Account for setting a same icon helper

g_set_object() will take care of ref'ing before destroying the previous
instance, which might actually be the same pointer.
Comment 9 Matthias Clasen 2015-07-07 03:44:07 UTC
Review of attachment 306955 [details] [review]:

yes
Comment 10 Carlos Garnacho 2015-07-07 09:25:11 UTC
Attachment 306955 [details] pushed as dadb275 - gtkdnd: Account for setting a same icon helper
Comment 11 Milan Crha 2015-07-13 13:00:25 UTC
*** Bug 752282 has been marked as a duplicate of this bug. ***