Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(630)

Issue 839253002: Gracefully handle mismatched drag ended notifications. (Closed)

Created:
5 years, 11 months ago by sof
Modified:
5 years, 4 months ago
CC:
blink-reviews, pkotwicz
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Gracefully 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -1 line) Patch
A LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html View 1 2 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/drag-extra-mouse-down-no-crash-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/page/EventHandler.cpp View 1 1 chunk +19 lines, -1 line 0 comments Download

Messages

Total messages: 26 (3 generated)
sof
Please take a look. (Continuing on from https://codereview.chromium.org/839073004/ )
5 years, 11 months ago (2015-01-09 19:59:01 UTC) #2
sof
+dcheng
5 years, 11 months ago (2015-01-12 07:03:36 UTC) #4
jochen (gone - plz use gerrit)
lgtm https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html File LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html (right): https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html#newcode1 LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html:1: <p> <!DOCTYPE html>
5 years, 11 months ago (2015-01-12 12:20:25 UTC) #5
Rick Byers
Thanks, I like this a lot better than cutting this off in eventSender. LGTM
5 years, 11 months ago (2015-01-12 22:22:54 UTC) #6
sof
Thanks for the reviews, here&there. https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html File LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html (right): https://codereview.chromium.org/839253002/diff/20001/LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html#newcode1 LayoutTests/fast/events/drag-extra-mouse-down-no-crash.html:1: <p> On 2015/01/12 12:20:25, ...
5 years, 11 months ago (2015-01-12 22:27:56 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/839253002/40001
5 years, 11 months ago (2015-01-12 22:28:27 UTC) #9
dcheng
https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp#newcode3222 Source/core/page/EventHandler.cpp:3222: if (!dragState().m_dragDataTransfer) It's not clear to me that this ...
5 years, 11 months ago (2015-01-12 22:35:48 UTC) #10
sof
https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp#newcode3222 Source/core/page/EventHandler.cpp:3222: if (!dragState().m_dragDataTransfer) On 2015/01/12 22:35:48, dcheng wrote: > It's ...
5 years, 11 months ago (2015-01-12 22:47:53 UTC) #11
dcheng
On 2015/01/12 at 22:47:53, sigbjornf wrote: > https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/839253002/diff/20001/Source/core/page/EventHandler.cpp#newcode3222 ...
5 years, 11 months ago (2015-01-12 22:52:29 UTC) #12
dcheng
Sorry, I'm going to not lgtm this patch because I don't feel comfortable with this ...
5 years, 11 months ago (2015-01-13 00:15:07 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188256
5 years, 11 months ago (2015-01-13 00:41:43 UTC) #14
sof
On 2015/01/13 00:15:07, dcheng wrote: > Sorry, I'm going to not lgtm this patch because ...
5 years, 11 months ago (2015-01-13 06:20:21 UTC) #15
sof
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/841533004/ by sigbjornf@opera.com. ...
5 years, 11 months ago (2015-01-13 06:22:45 UTC) #16
dcheng
No worries, I know it was pretty late when I replied and didn't expect you ...
5 years, 11 months ago (2015-01-13 08:43:07 UTC) #17
Rick Byers
Sorry for the extra chaos on this issue. I contemplated saying we should wait for ...
5 years, 11 months ago (2015-01-13 17:58:14 UTC) #18
dcheng
OK, so looking at this more, some of my original assumptions are incorrect. It appears ...
5 years, 11 months ago (2015-01-13 18:39:25 UTC) #19
Rick Byers
On 2015/01/13 18:39:25, dcheng wrote: > OK, so looking at this more, some of my ...
5 years, 11 months ago (2015-01-13 19:10:47 UTC) #20
dcheng
I think this is a clear won't fix then. This prevents the crash, but doesn't ...
5 years, 11 months ago (2015-01-13 20:52:16 UTC) #21
sof
On 2015/01/13 20:52:16, dcheng wrote: > I think this is a clear won't fix then. ...
5 years, 11 months ago (2015-01-13 21:22:04 UTC) #22
Rick Byers
On 2015/01/13 20:52:16, dcheng wrote: > I think this is a clear won't fix then. ...
5 years, 11 months ago (2015-01-13 21:33:08 UTC) #23
dcheng
I'd like to say that the last time I had a bug like this (invalid ...
5 years, 11 months ago (2015-01-13 21:45:07 UTC) #24
Rick Byers
On 2015/01/13 21:45:07, dcheng wrote: > I'd like to say that the last time I ...
5 years, 11 months ago (2015-01-13 21:55:12 UTC) #25
dcheng
5 years, 11 months ago (2015-01-13 22:00:14 UTC) #26
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.

Powered by Google App Engine
This is Rietveld 408576698