Reset DragState::m_dragSrc when mouse move doesn't meet drag threshold.

Reset DragState::m_dragSrc when mouse move doesn't meet drag threshold.

The rest of blink assumes that the DragState is completely valid if
DragState::m_dragSrc is not null. However, MouseEventManger::handleDrag
has a code path that sets DragState::m_dragSrc, but does not call
tryStartDrag(), which sets DragState::m_dataTransfer. This creates the
opportunity for crashes, as DragState::m_dataTransfer is initialized to
null when a renderer is created.

BUG=677916
Review-Url: https://codereview.chromium.org/2610023002
Cr-Commit-Position: refs/heads/master@{#444252}
Committed: https://chromium.googlesource.com/chromium/src/+/441df0543f6f4029f73068dec974d4dac47f6a1f

[pwnall, 2017-01-03 22:44:53]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 22:45:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-01-03 22:53:29]

pwnall@chromium.org changed reviewers:
+ dcheng@chromium.org

[pwnall, 2017-01-03 22:53:30]

PTAL?

The bug has stack traces, and a description of why I think the crash is
happening. Does this fix look reasonable to you?

Thank you very much!

[commit-bot: I haz the power, 2017-01-04 00:20:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 00:20:45]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-04 19:11:11]

The slightly surprising part (to me) is that we'd get a drag ended signal here
at all: if we don't exceed the drag threshhold, we shouldn't start a drag at
all. And if we don't start a drag, how do we get a drag end signal?

[pwnall, 2017-01-04 22:37:01]

On 2017/01/04 19:11:11, dcheng wrote:
> The slightly surprising part (to me) is that we'd get a drag ended signal here
> at all: if we don't exceed the drag threshhold, we shouldn't start a drag at
> all. And if we don't start a drag, how do we get a drag end signal?

I don't understand the Chromium-side code quite yet, but I was thinking of a
scenario where a navigation happens while a drag is in progress, along the lines
of http://crbug.com/660932

Also, it seems to me that it'd be easier to reason the code if we maintain the
invariant that m_dragSrc is nullptr as long as no drag is happening.

[pwnall, 2017-01-12 01:19:42]

On 2017/01/04 22:37:01, pwnall wrote:
> On 2017/01/04 19:11:11, dcheng wrote:
> > The slightly surprising part (to me) is that we'd get a drag ended signal
here
> > at all: if we don't exceed the drag threshhold, we shouldn't start a drag at
> > all. And if we don't start a drag, how do we get a drag end signal?
> 
> I don't understand the Chromium-side code quite yet, but I was thinking of a
> scenario where a navigation happens while a drag is in progress, along the
lines
> of http://crbug.com/660932
> 
> Also, it seems to me that it'd be easier to reason the code if we maintain the
> invariant that m_dragSrc is nullptr as long as no drag is happening.

I've been digging around the Windows-specific drag-and-drop code on the browser
side on and off (due to interruptions), and I haven't figured out the cause yet.
Would you be OK with landing this fix speculatively and merging to M56, and
seeing if the crashes go away?

[dcheng, 2017-01-16 10:45:35]

On 2017/01/12 01:19:42, pwnall wrote:
> On 2017/01/04 22:37:01, pwnall wrote:
> > On 2017/01/04 19:11:11, dcheng wrote:
> > > The slightly surprising part (to me) is that we'd get a drag ended signal
> here
> > > at all: if we don't exceed the drag threshhold, we shouldn't start a drag
at
> > > all. And if we don't start a drag, how do we get a drag end signal?
> > 
> > I don't understand the Chromium-side code quite yet, but I was thinking of a
> > scenario where a navigation happens while a drag is in progress, along the
> lines
> > of http://crbug.com/660932
> > 
> > Also, it seems to me that it'd be easier to reason the code if we maintain
the
> > invariant that m_dragSrc is nullptr as long as no drag is happening.
> 
> I've been digging around the Windows-specific drag-and-drop code on the
browser
> side on and off (due to interruptions), and I haven't figured out the cause
yet.
> Would you be OK with landing this fix speculatively and merging to M56, and
> seeing if the crashes go away?

On 2017/01/12 01:19:42, pwnall wrote:
> On 2017/01/04 22:37:01, pwnall wrote:
> > On 2017/01/04 19:11:11, dcheng wrote:
> > > The slightly surprising part (to me) is that we'd get a drag ended signal
> here
> > > at all: if we don't exceed the drag threshhold, we shouldn't start a drag
at
> > > all. And if we don't start a drag, how do we get a drag end signal?
> > 
> > I don't understand the Chromium-side code quite yet, but I was thinking of a
> > scenario where a navigation happens while a drag is in progress, along the
> lines
> > of http://crbug.com/660932
> > 
> > Also, it seems to me that it'd be easier to reason the code if we maintain
the
> > invariant that m_dragSrc is nullptr as long as no drag is happening.
> 
> I've been digging around the Windows-specific drag-and-drop code on the
browser
> side on and off (due to interruptions), and I haven't figured out the cause
yet.
> Would you be OK with landing this fix speculatively and merging to M56, and
> seeing if the crashes go away?

I don't think it's unreasonable to merge the fix itelf, but I feel like the test
is executing what should be an impossible code path (without calling
tryStartDrag, we should never start an OS drag, which means we should never get
a drag source ended event). Is it possible to make this CL reflect that?

[pwnall, 2017-01-16 18:45:50]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 18:46:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 20:34:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 20:34:09]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-01-16 21:07:11]

On 2017/01/16 10:45:35, dcheng wrote:
> I don't think it's unreasonable to merge the fix itelf, but I feel like the
test
> is executing what should be an impossible code path (without calling
> tryStartDrag, we should never start an OS drag, which means we should never
get
> a drag source ended event). Is it possible to make this CL reflect that?

Is this (updated CL) what you meant, or would you like the test removed? I hope
that the comment change makes it not be misleading.

Thank you for taking the time to review this!

[dcheng, 2017-01-17 22:24:11]

LGTM

https://codereview.chromium.org/2610023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2610023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:279: // could occur
before a drag operation is started.
Let's file a followup bug for this (or make sure we track it in the original)
and note that here as a TODO: if we are violating this condition, this is a
pretty big violation of the invariants that the renderer assumes the browser
will keep.

[pwnall, 2017-01-18 00:15:49]

Patchset #3 (id:40001) has been deleted

[pwnall, 2017-01-18 00:16:30]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 00:17:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-01-18 00:17:50]

Thank you very much for the review!

https://codereview.chromium.org/2610023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2610023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:279: // could occur
before a drag operation is started.
On 2017/01/17 22:24:11, dcheng wrote:
> Let's file a followup bug for this (or make sure we track it in the original)
> and note that here as a TODO: if we are violating this condition, this is a
> pretty big violation of the invariants that the renderer assumes the browser
> will keep.

Done.

I'll also monitor the crash metrics to make sure this fix makes the crash go
away.

[commit-bot: I haz the power, 2017-01-18 00:19:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:20:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pwnall, 2017-01-18 01:10:24]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 01:11:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 02:33:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 02:33:08]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-01-18 02:47:17]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2017-01-18 02:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2610023002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-01-18 02:47:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 02:51:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484707637199960,
"parent_rev": "00320b068a7e061eb3634293c19a0401357fa66a", "commit_rev":
"441df0543f6f4029f73068dec974d4dac47f6a1f"}

[commit-bot: I haz the power, 2017-01-18 02:55:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484707637199960,
"parent_rev": "00320b068a7e061eb3634293c19a0401357fa66a", "commit_rev":
"441df0543f6f4029f73068dec974d4dac47f6a1f"}

[commit-bot: I haz the power, 2017-01-18 02:56:20]

Description was changed from

==========
Reset DragState::m_dragSrc when mouse move doesn't meet drag threshold.

The rest of blink assumes that the DragState is completely valid if
DragState::m_dragSrc is not null. However, MouseEventManger::handleDrag
has a code path that sets DragState::m_dragSrc, but does not call
tryStartDrag(), which sets DragState::m_dataTransfer. This creates the
opportunity for crashes, as DragState::m_dataTransfer is initialized to
null when a renderer is created.

BUG=677916
==========

to

==========
Reset DragState::m_dragSrc when mouse move doesn't meet drag threshold.

The rest of blink assumes that the DragState is completely valid if
DragState::m_dragSrc is not null. However, MouseEventManger::handleDrag
has a code path that sets DragState::m_dragSrc, but does not call
tryStartDrag(), which sets DragState::m_dataTransfer. This creates the
opportunity for crashes, as DragState::m_dataTransfer is initialized to
null when a renderer is created.

BUG=677916

Review-Url: https://codereview.chromium.org/2610023002
Cr-Commit-Position: refs/heads/master@{#444252}
Committed:
https://chromium.googlesource.com/chromium/src/+/441df0543f6f4029f73068dec974...
==========

[commit-bot: I haz the power, 2017-01-18 02:56:22]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/441df0543f6f4029f73068dec974...