Bug 217482 – [GTK] Crash in WebKit::DropTarget::drop

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

I haven't seen this particular drag-n-drop bug before, so it's probably new in 2.30?

Thread 1 (Thread 0x7f3eeff62c80 (LWP 3523)):
#0  __GI_raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:49
        set = {__val = {0, 93856813486576, 0, 72057594037927988, 0, 140721518124432, 4627167142146474036, 16801918529473529088, 93856870608272, 93856813042704, 0, 93857127715200, 52, 4294967300, 4, 139908447935547}}
        pid = <optimized out>
        tid = <optimized out>
        ret = <optimized out>
#1  0x00007f3ef44a18a4 in __GI_abort () at abort.c:79
        save_stage = 1
        act = {__sigaction_handler = {sa_handler = 0x7ffc48199f5c, sa_sigaction = 0x7ffc48199f5c}, sa_mask = {__val = {139908453178748, 140721518124900, 93856824678096, 140721518124896, 0, 139906547279360, 139908453179008, 0, 16801918529473529088, 140721518125104, 93856817365984, 93856867505936, 139908422963647, 139908027480608, 1095216660830, 0}}, sa_flags = -699218688, sa_restorer = 0x10}
        sigs = {__val = {32, 139908449167371, 139908449106432, 139908453179171, 4644407484470001664, 350, 93856825400816, 93856817365984, 93856817365984, 139908449167371, 324, 93856824678096, 0, 0, 140721518124896, 139908449167371}}
#2  0x00007f3ef6437f1b in WTF::Optional<WebCore::SelectionData>::value() & (this=0x7f3ee0477a40) from /lib64/libwebkit2gtk-4.0.so.37
No locals.
#3  WebKit::DropTarget::drop (this=0x7f3ee0477a20, position=..., time=0) at ../Source/WebKit/UIProcess/API/gtk/DropTargetGtk3.cpp:274
        page = 0x7f3e880d6600
        flags = <optimized out>
        dragData = {m_clientPosition = {m_x = 0, m_y = 0}, m_globalPosition = {m_x = -1052813552, m_y = 21852}, m_platformDragData = 0x9000000144, m_draggingSourceOperationMask = {m_storage = 144 '\220'}, m_applicationFlags = {m_storage = 160 '\240'}, m_fileNames = {<WTF::VectorBuffer<WTF::String, 0, WTF::FastMalloc>> = {<WTF::VectorBufferBase<WTF::String, WTF::FastMalloc>> = {m_buffer = 0x555cc14b09d0, m_capacity = 0, m_size = 0}, <No data fields>}, <No data fields>}, m_dragDestinationActionMask = {m_storage = 16 '\020'}}
#4  0x00007f3ef68f3f6d in operator() (__closure=0x0, userData=<optimized out>, time=<optimized out>, y=<optimized out>, x=<optimized out>, context=<optimized out>) at DerivedSources/ForwardingHeaders/WebCore/IntPoint.h:67
        drop = <optimized out>
#5  _FUN () at ../Source/WebKit/UIProcess/API/gtk/DropTargetGtk3.cpp:77
No locals.
#6  0x00007f3ef9f01cf6 in _gtk_marshal_BOOLEAN__OBJECT_INT_INT_UINTv (closure=0x555cc14723b0, return_value=0x7ffc4819a190, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, param_types=0x555cbe057870) at gtkmarshalers.c:884
        cc = <optimized out>
        data1 = 0x555cc13f5710
        data2 = <optimized out>
        callback = 0x7f3ef68f3f10 <_FUN(GtkWidget*, GdkDragContext*, gint, gint, guint, gpointer)>
        v_return = <optimized out>
        arg0 = 0x555cbe0360e0
        arg1 = 8
        arg2 = 582
        arg3 = 0
        args_copy = {{gp_offset = 48, fp_offset = 48, overflow_arg_area = 0x7ffc4819a360, reg_save_area = 0x7ffc4819a270}}
        __func__ = "_gtk_marshal_BOOLEAN__OBJECT_INT_INT_UINTv"
#7  0x00007f3ef969a260 in _g_closure_invoke_va (param_types=0x555cbe057870, n_params=<optimized out>, args=0x7ffc4819a240, instance=0x555cc13f5710, return_value=0x7ffc4819a190, closure=0x555cc14723b0) at ../gobject/gclosure.c:873
        marshal = <optimized out>
        marshal_data = <optimized out>
        in_marshal = 0
        real_closure = 0x555cc1472390
        real_closure = <optimized out>
        __func__ = <optimized out>
        _g_boolean_var_ = <optimized out>
        marshal = <optimized out>
        marshal_data = <optimized out>
        in_marshal = <optimized out>
        _g_boolean_var_ = <optimized out>
        cunion = <optimized out>
        new_int = <optimized out>
        old_int = <optimized out>
        success = <optimized out>
        tmp = <optimized out>
        gaicae_oldval = <optimized out>
        cunion = <optimized out>
        new_int = <optimized out>
        old_int = <optimized out>
        success = <optimized out>
        tmp = <optimized out>
        gaicae_oldval = <optimized out>
#8  g_signal_emit_valist (instance=instance@entry=0x555cc13f5710, signal_id=signal_id@entry=113, detail=detail@entry=0, var_args=var_args@entry=0x7ffc4819a240) at ../gobject/gsignal.c:3403
        return_accu = 0x7ffc4819a190
        accu = {g_type = 20, data = {{v_int = 0, v_uint = 0, v_long = 0, v_ulong = 0, v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, v_pointer = 0x0}, {v_int = 0, v_uint = 0, v_long = 0, v_ulong = 0, v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, v_pointer = 0x0}}}
        accumulator = 0x555cbe086260
        emission = {next = 0x0, instance = 0x555cc13f5710, ihint = {signal_id = 113, detail = 0, run_type = G_SIGNAL_RUN_FIRST}, state = EMISSION_RUN, chain_type = 93856814379296}
        signal_id = 113
        instance_type = 93856814379296
        emission_return = {g_type = 20, data = {{v_int = 0, v_uint = 0, v_long = 0, v_ulong = 0, v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, v_pointer = 0x0}, {v_int = 0, v_uint = 0, v_long = 0, v_ulong = 0, v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, v_pointer = 0x0}}}
        rtype = 20
        static_scope = 0
        fastpath_handler = <optimized out>
        closure = <optimized out>
        run_type = <optimized out>
        hlist = <optimized out>
        l = <optimized out>
        fastpath = 1
        instance_and_params = <optimized out>
        signal_return_type = <optimized out>
        param_values = <optimized out>
        node = <optimized out>
        i = <optimized out>
        n_params = <optimized out>
        __func__ = "g_signal_emit_valist"
#9  0x00007f3ef969a599 in g_signal_emit_by_name (instance=0x555cc13f5710, detailed_signal=0x7f3ef9f46525 "drag-drop") at ../gobject/gsignal.c:3590
        var_args = {{gp_offset = 16, fp_offset = 48, overflow_arg_area = 0x7ffc4819a360, reg_save_area = 0x7ffc4819a270}}
        detail = 0
        signal_id = 113
        itype = 93856814379296
        __func__ = "g_signal_emit_by_name"
#10 0x00007f3ef9edf230 in gtk_drag_dest_drop (widget=widget@entry=0x555cc13f5710, context=context@entry=0x555cbe0360e0, x=324, y=144, time=time@entry=0) at gtkdnd.c:1674
        retval = -101326739
        site = 0x555cc0258090
        info = <optimized out>
        __func__ = "gtk_drag_dest_drop"
#11 0x00007f3ef9d67aab in gtk_drag_find_widget (callback=0x7f3ef9edf170 <gtk_drag_dest_drop>, time=0, y=<optimized out>, x=<optimized out>, info=0x555cbfa5a030, context=0x555cbe0360e0, widget=0x555cc13f5710) at gtkdnd.c:1270
        parent = 0x0
        hierarchy = 0x555ccf22cc40
        found = 0
#12 _gtk_drag_dest_handle_event (event=0x555ccf2e1d10, toplevel=<optimized out>) at gtkdnd.c:1091
        window = <optimized out>
        tx = 0
        ty = 0
        found = <optimized out>
        info = 0x555cbfa5a030
        context = 0x555cbe0360e0
        info = <optimized out>
        context = <optimized out>
        __func__ = <optimized out>
        _g_boolean_var_ = <optimized out>
        _g_boolean_var_ = <optimized out>
        window = <optimized out>
        tx = <optimized out>
        ty = <optimized out>
        found = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
#13 gtk_main_do_event (event=0x555ccf2e1d10) at gtkmain.c:1938
        grab_widget = <optimized out>
        window_group = 0x7ffc4819a428
        rewritten_event = <optimized out>
        device = <optimized out>
        tmp_list = <optimized out>
        event_widget = <optimized out>
        topmost_widget = <optimized out>
        event_widget = <optimized out>
        grab_widget = <optimized out>
        topmost_widget = <optimized out>
        window_group = <optimized out>
        rewritten_event = <optimized out>
        device = <optimized out>
        tmp_list = <optimized out>
        __func__ = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        window = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        mnemonics_visible = <optimized out>
        window = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
#14 gtk_main_do_event (event=<optimized out>) at gtkmain.c:1690
        event_widget = <optimized out>
        grab_widget = <optimized out>
        topmost_widget = <optimized out>
        window_group = <optimized out>
        rewritten_event = <optimized out>
        device = <optimized out>
        tmp_list = <optimized out>
        __func__ = "gtk_main_do_event"
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        window = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
        mnemonics_visible = <optimized out>
        window = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
#15 0x00007f3ef9a56043 in _gdk_event_emit (event=0x555ccf2e1d10) at gdkevents.c:73
No locals.
#16 _gdk_event_emit (event=0x555ccf2e1d10) at gdkevents.c:67
No locals.
#17 0x00007f3ef9abc172 in gdk_event_source_dispatch (base=base@entry=0x555cbe040b50, callback=<optimized out>, data=<optimized out>) at gdkeventsource.c:124
        source = 0x555cbe040b50
        display = <optimized out>
        event = 0x555ccf2e1d10
#18 0x00007f3ef958cff7 in g_main_dispatch (context=0x555cbe01e120) at ../glib/gmain.c:3325
        dispatch = <optimized out>
        prev_source = 0x0
        begin_time_nsec = 0
        was_in_call = <optimized out>
        user_data = 0x0
        callback = 0x0
        cb_funcs = 0x0
        cb_data = 0x0
        need_destroy = <optimized out>
        source = 0x555cbe040b50
        current = 0x555cbe01e1e0
        i = 0
        current = <optimized out>
        i = <optimized out>
        __func__ = <optimized out>
        source = <optimized out>
        _g_boolean_var_ = <optimized out>
        was_in_call = <optimized out>
        user_data = <optimized out>
        callback = <optimized out>
        cb_funcs = <optimized out>
        cb_data = <optimized out>
        need_destroy = <optimized out>
        dispatch = <optimized out>
        prev_source = <optimized out>
        begin_time_nsec = <optimized out>
        _g_boolean_var_ = <optimized out>
#19 g_main_context_dispatch (context=0x555cbe01e120) at ../glib/gmain.c:4016
No locals.
#20 0x00007f3ef95ddbb8 in g_main_context_iterate.constprop.0 (context=context@entry=0x555cbe01e120, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4092
        max_priority = 2147483647
        timeout = 310
        some_ready = 1
        nfds = <optimized out>
        allocated_nfds = <optimized out>
        fds = 0x555cd0713fa0
#21 0x00007f3ef958a41f in g_main_context_iteration (context=0x555cbe01e120, may_block=1) at ../glib/gmain.c:4157
        retval = <optimized out>
#22 0x00007f3ef97983e5 in g_application_run (application=0x555cbe014770, argc=1209640436, argv=<optimized out>) at ../gio/gapplication.c:2559
        arguments = 0x555cbe188710
        status = 0
        context = 0x555cbe01e120
        acquired_context = <optimized out>
        __func__ = "g_application_run"
#23 0x0000555cbc0bcc2b in main (argc=<optimized out>, argv=<optimized out>) at ../src/ephy-main.c:431
        option_context = <optimized out>
        option_group = <optimized out>
        error = 0x0
        user_time = 140364
        arbitrary_url = <optimized out>
        ctx = <optimized out>
        mode = <optimized out>
        status = <optimized out>
        flags = <optimized out>
        desktop_info = <optimized out>

[Michael Catanzaro]

OK here's a guess: maybe (1) user starts drag, (2) user leaves window, m_leaveTimer starts running, (3) user starts a new drag, m_leaveTimer still running, (4) m_leaveTimer fires, unsets m_selectionData etc., (5) user releases button, triggering drop, (6) crash.

It seems a little unlikely, because m_leaveTimer is stopped in DropTarget::accept, so the user would have to finish the drop before the source application sends its drag data offer. But that's actually possible, right?

I see we have, in DropTarget::accept:

    if (m_leaveTimer.isActive()) {
        m_leaveTimer.stop();
        leaveTimerFired();
    }

But that's not soon enough, right? It belongs in DropTarget::enter?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> OK here's a guess: maybe (1) user starts drag, (2) user leaves window,
> m_leaveTimer starts running, (3) user starts a new drag, m_leaveTimer still
> running, (4) m_leaveTimer fires, unsets m_selectionData etc., (5) user
> releases button, triggering drop, (6) crash.
> 
> It seems a little unlikely, because m_leaveTimer is stopped in
> DropTarget::accept, so the user would have to finish the drop before the
> source application sends its drag data offer. But that's actually possible,
> right?

It's actually called the first time drag-motion is received, so that is a little more plausible. But it still relies on that timer not firing immediately. My expectation is that the timer created with 0_s timeout would run on next iteration of the main loop, so it seems very unlikely... but I haven't looked into how Timer is implemented.

Notably, however, DropTarget::leaveTimerFired is actually prepared for m_selectionData to be nullopt! But DropTarget::drop is not. So if nothing else, that is inconsistent.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #1)
> OK here's a guess: maybe (1) user starts drag, (2) user leaves window,
> m_leaveTimer starts running, (3) user starts a new drag, m_leaveTimer still
> running, (4) m_leaveTimer fires, unsets m_selectionData etc., (5) user
> releases button, triggering drop, (6) crash.
> 
> It seems a little unlikely, because m_leaveTimer is stopped in
> DropTarget::accept, so the user would have to finish the drop before the
> source application sends its drag data offer. But that's actually possible,
> right?
> 
> I see we have, in DropTarget::accept:
> 
>     if (m_leaveTimer.isActive()) {
>         m_leaveTimer.stop();
>         leaveTimerFired();
>     }
> 
> But that's not soon enough, right? It belongs in DropTarget::enter?

accept happens before enter. accept starts reading the contents and after all data has been received then enter is called.

[Carlos Garcia Campos]

Was this under xorg or wayland?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #1)
> OK here's a guess: maybe (1) user starts drag, (2) user leaves window,
> m_leaveTimer starts running, (3) user starts a new drag, m_leaveTimer still
> running, (4) m_leaveTimer fires, unsets m_selectionData etc., (5) user
> releases button, triggering drop, (6) crash.
> 
> It seems a little unlikely, because m_leaveTimer is stopped in
> DropTarget::accept, so the user would have to finish the drop before the
> source application sends its drag data offer. But that's actually possible,
> right?
> 
> I see we have, in DropTarget::accept:
> 
>     if (m_leaveTimer.isActive()) {
>         m_leaveTimer.stop();
>         leaveTimerFired();
>     }
> 
> But that's not soon enough, right? It belongs in DropTarget::enter?

I've noticed a problem with this code, though. It resets the drop context and position set in drag-motion callback

[Carlos Garcia Campos]

Created attachment 413415 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #4)
> Was this under xorg or wayland?

Probably Wayland

[Michael Catanzaro]

Comment on attachment 413415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413415&action=review

> Source/WebKit/ChangeLog:9
> +        accept() to receive the drop context and position to ne set after leaving any previous operation.

ne -> be

> Source/WebKit/UIProcess/API/gtk/DropTargetGtk3.cpp:262
> +    // If we don't have data at this point, let leave continue.

If I understand this correctly, then how about: "If we don't have data at this point, allow the leave timer to fire, ending the drop operation."

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> (In reply to Carlos Garcia Campos from comment #4)
> > Was this under xorg or wayland?
> 
> Probably Wayland

Maybe not, looks like this was a downstream bug report rather than one I experienced myself.

[Carlos Garcia Campos]

Committed r269620: <https://trac.webkit.org/changeset/269620>