Summary: | [GTK] Crash in WebKit::DropTarget::drop | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, mcatanzaro | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
See Also: |
https://bugzilla.redhat.com/show_bug.cgi?id=1886523 https://bugs.webkit.org/show_bug.cgi?id=190787 |
||||||
Attachments: |
|
Description
Michael Catanzaro
2020-10-08 10:11:04 PDT
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? (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. (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. Was this under xorg or wayland? (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 Created attachment 413415 [details]
Patch
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 (In reply to Carlos Garcia Campos from comment #4) > Was this under xorg or wayland? Probably Wayland 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." (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. Committed r269620: <https://trac.webkit.org/changeset/269620> |