Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
839253002:
Gracefully handle mismatched drag ended notifications. (Closed)
DescriptionGracefully handle mismatched drag ended notifications.
Should the embedder end up signalling the end of a drag operation
via EventHandler::dragSourceEndedAt(), but Blink has initiated
another drag operation, for whatever reason or source, then catch
this and gracefully bail.
R=jochen,rbyers
BUG=445308
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188256
Patch Set 1 #Patch Set 2 : Ignore mismatched dragSourceEndedAt() invocations. #
Total comments: 4
Patch Set 3 : test tidying #
Messages
Total messages: 26 (3 generated)
sigbjornf@opera.com changed reviewers: + jochen@chromium.org, rbyers@chromium.org
Please take a look. (Continuing on from https://codereview.chromium.org/839073004/ )
sigbjornf@opera.com changed reviewers: + dcheng@chromium.org
+dcheng
lgtm https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/... File LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html (right): https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html:1: <p> <!DOCTYPE html>
Thanks, I like this a lot better than cutting this off in eventSender. LGTM
Thanks for the reviews, here&there. https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/... File LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html (right): https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html:1: <p> On 2015/01/12 12:20:25, jochen (slow) wrote: > <!DOCTYPE html> Done.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/839253002/40001
https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:3222: if (!dragState().m_dragDataTransfer) It's not clear to me that this fix/description is correct. I don't see any code that actually clears m_dragDataTransfer. So it seems a precondition of this triggering is that this is the first drag on a page. Then it seems like one of two conditions must be true: 1. In between m_dragSrc being set in EventHandler::handleDrag and tryStartDrag(), we somehow spin a nested message loop and get a dragSourceEndedAt() from the browser. I don't see anything that could run a nested message loop in that short block though. 2. We return after a potential drag source has been selected, and spin the message loop as the user drags, but the potential drag ends before we ever exceed the drag hysteresis (and therefore m_dragDataTransfer is still null).
https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:3222: if (!dragState().m_dragDataTransfer) On 2015/01/12 22:35:48, dcheng wrote: > It's not clear to me that this fix/description is correct. I don't see any code > that actually clears m_dragDataTransfer. So it seems a precondition of this > triggering is that this is the first drag on a page. > > Then it seems like one of two conditions must be true: > 1. In between m_dragSrc being set in EventHandler::handleDrag and > tryStartDrag(), we somehow spin a nested message loop and get a > dragSourceEndedAt() from the browser. I don't see anything that could run a > nested message loop in that short block though. > > 2. We return after a potential drag source has been selected, and spin the > message loop as the user drags, but the potential drag ends before we ever > exceed the drag hysteresis (and therefore m_dragDataTransfer is still null). The testcase sorts under 2, I think.
On 2015/01/12 at 22:47:53, sigbjornf wrote: > https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHa... > Source/core/page/EventHandler.cpp:3222: if (!dragState().m_dragDataTransfer) > On 2015/01/12 22:35:48, dcheng wrote: > > It's not clear to me that this fix/description is correct. I don't see any code > > that actually clears m_dragDataTransfer. So it seems a precondition of this > > triggering is that this is the first drag on a page. > > > > Then it seems like one of two conditions must be true: > > 1. In between m_dragSrc being set in EventHandler::handleDrag and > > tryStartDrag(), we somehow spin a nested message loop and get a > > dragSourceEndedAt() from the browser. I don't see anything that could run a > > nested message loop in that short block though. > > > > 2. We return after a potential drag source has been selected, and spin the > > message loop as the user drags, but the potential drag ends before we ever > > exceed the drag hysteresis (and therefore m_dragDataTransfer is still null). > > The testcase sorts under 2, I think. Not really--if my interpretation is correct, there's nothing happening concurrently. There should be no mouse down in the middle of a drag--it seems like this should happen for a plain old drag that doesn't exceed the drag hysteresis.
Sorry, I'm going to not lgtm this patch because I don't feel comfortable with this patch landing as is without my questions answered.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188256
Message was sent while issue was closed.
On 2015/01/13 00:15:07, dcheng wrote: > Sorry, I'm going to not lgtm this patch because I don't feel comfortable with > this patch landing as is without my questions answered. Sorry, I wasn't willfully ignoring your input (albeit late arriving); did you go through the exchange on https://codereview.chromium.org/839073004/ that worked through where this change would be better placed? This isn't papering over some bug in drag event handling on the Blink side, but coping it with what happens if mouse events arrive out of turn from the embedder. In this case, a file drag is initiated and a mouse down is then sent (followed by, as you suspected, limited mouse movement, so the hysteresis threshold applies.) That nested start of a "near drag" is done without the embedder being in on it, so when the mouse up arrives, it wants to finalize the initial file drag. At which point we end up in the code changed by the CL. This is all EventSender-driven, so you might say this won't happen in RL and that Blink really shouldn't/cannot handle an illbehaved embedder in any case, so shouldn't be trying (at which point we're at https://codereview.chromium.org/839073004/ ). That's the context and reasoning behind this CL; does that explain a bit and/or address your concerns in some way?
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/841533004/ by sigbjornf@opera.com. The reason for reverting is: Questions arose on the review after the CL entered CQ (and the author went to bed). Reverting until we're all aligned..
Message was sent while issue was closed.
No worries, I know it was pretty late when I replied and didn't expect you to be around; I thought a not lgtm would cancel the CQ (but now I know!). I will look at this first thing tomorrow morning. Sorry it's a bit late on my end =/
Sorry for the extra chaos on this issue. I contemplated saying we should wait for dcheng@ to weigh in (he's the expert on blink DnD code), but was hoping it was simple enough we didn't need to block on him. My bad... Daniel, my high-level opinion on the other review was that we don't necessarily need to invest a ton in making blink perfectly handle unexpected / invalid event stream combinations, but we should make sure it doesn't crash or otherwise have a catastrophic behavior (rather than change event_sender to force it to conform to expected sequences, and hope that chromium won't deviate).
OK, so looking at this more, some of my original assumptions are incorrect. It appears that you are correct about the embedder interleaving events for different drags. However, IMO we should fix the embedder to not do this--in the past, we've always made sure that the embedder provides Blink with a consistent view of the world (for example, https://chromium.googlesource.com/chromium/chromium/+/4e693dd4033eb7b76787d3d..., search for DelayedOnDragSourceDrop). Drag and drop logic (and event handling in general) is already far too complicated. Forcing it to handle invalid event streams is just out of scope of what it should have to deal with. +pkotwicz, who has worked with a lot of the embedder side of this, in case he has any ideas about patches that could have introduced a regression (?) like this.
On 2015/01/13 18:39:25, dcheng wrote: > OK, so looking at this more, some of my original assumptions are incorrect. It > appears that you are correct about the embedder interleaving events for > different drags. > > However, IMO we should fix the embedder to not do this--in the past, we've > always made sure that the embedder provides Blink with a consistent view of the > world (for example, > https://chromium.googlesource.com/chromium/chromium/+/4e693dd4033eb7b76787d3d..., > search for DelayedOnDragSourceDrop). > > Drag and drop logic (and event handling in general) is already far too > complicated. Forcing it to handle invalid event streams is just out of scope of > what it should have to deal with. > > +pkotwicz, who has worked with a lot of the embedder side of this, in case he > has any ideas about patches that could have introduced a regression (?) like > this. I don't think we have any evidence of a regression in chromium here. The bug was triggered by clusterfuzz using event sender to send an invalid event stream. sof@'s first fix was to change event_sender to make it reject such invalid event streams, but I argued that in general we want event_sender to be able to generate any possible event stream (eg. we use it to test blink fixes for how it copes with OS / chromium input bugs - see my other comments in https://codereview.chromium.org/839073004/). Do you disagree?
I think this is a clear won't fix then. This prevents the crash, but doesn't prevent other renderer state from ending up potentially garbled. If we interleave drags, who knows what other state is getting stomped on. I think it's better to just crash.
On 2015/01/13 20:52:16, dcheng wrote: > I think this is a clear won't fix then. > > This prevents the crash, but doesn't prevent other renderer state from ending up > potentially garbled. If we interleave drags, who knows what other state is > getting stomped on. I think it's better to just crash. Is it equally clear to you that event_sender should be able to inject such ill-formed event sequences?
On 2015/01/13 20:52:16, dcheng wrote: > I think this is a clear won't fix then. > > This prevents the crash, but doesn't prevent other renderer state from ending up > potentially garbled. If we interleave drags, who knows what other state is > getting stomped on. I think it's better to just crash. I agree crashing immediately is better than limping along with invalid state and potentially crashing/corrupting later. Is there an easy way perhaps to prevent the state from getting garbled up in the first place (detect/ignore the invalid event sequence sooner)?
I'd like to say that the last time I had a bug like this (invalid event sequence from EventSender), it was closed as won't fix... but I can't find it.
On 2015/01/13 21:45:07, dcheng wrote: > I'd like to say that the last time I had a bug like this (invalid event sequence > from EventSender), it was closed as won't fix... but I can't find it. Well we should be doing something here because this bug is causing noise for clusterfuzz. Random input event sequence fuzzing is kinda a nice test facility to have :-)
On 2015/01/13 at 21:55:12, rbyers wrote: > On 2015/01/13 21:45:07, dcheng wrote: > > I'd like to say that the last time I had a bug like this (invalid event sequence > > from EventSender), it was closed as won't fix... but I can't find it. > > Well we should be doing something here because this bug is causing noise for clusterfuzz. Random input event sequence fuzzing is kinda a nice test facility to have :-) Yeah, let me keep looking and see if I can figure out what I did last time. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
