Ignore left-button eventSender.mouseDown() uses while dragging.

Ignore left-button eventSender.mouseDown() uses while dragging.

To avoid confusing the drag&drop implementation overly, if an
eventSender-initiated drag operation is already in progress, ignore
uses of eventSender.mouseDown() for left button presses.

R=
BUG=445308

[sof, 2015-01-09 16:50:29]

sigbjornf@opera.com changed reviewers:
+ jochen@chromium.org, rbyers@chromium.org

[sof, 2015-01-09 16:50:30]

Please take a look.

Allowing mouseDown() while a file drag has been initiated via eventSender, just
makes Blink very confused when the drag comes to an eventual end.

A layout test that exercises non-crashing behavior will be added by
https://codereview.chromium.org/839253002/

[Rick Byers, 2015-01-09 17:01:08]

On 2015/01/09 16:50:30, sof wrote:
> Please take a look.
> 
> Allowing mouseDown() while a file drag has been initiated via eventSender,
just
> makes Blink very confused when the drag comes to an eventual end.
> 
> A layout test that exercises non-crashing behavior will be added by
> https://codereview.chromium.org/839253002/

Are you sure this crash can't be provoked in scenarios outside of event sender? 
Eg. maybe if I start dragging a file using touch drag and drop, and then do a
mouse button down.  Or what if I'm dragging a file with one mouse device then a
second mouse device sends a left button down event?

If it's at all reasonable the fix the underlying issue with confusion in blink
then I'd prefer that to an event_sender-specific fix.  It's nice to have
clusterfuzz be able to generate arbitrary overlapped input through eventSender
to simulate potential real-world scenarios.  But if we reasonably believe that
such a scenario can never happen in practice (eg. maybe by a comment in event
sender pointing at the code in chromium/blink that implements such a restriction
on real input events), then it's reasonable to add code like this to prevent it
in event_sender.

[sof, 2015-01-09 17:23:43]

On 2015/01/09 17:01:08, Rick Byers wrote:
> On 2015/01/09 16:50:30, sof wrote:
> > Please take a look.
> > 
> > Allowing mouseDown() while a file drag has been initiated via eventSender,
> just
> > makes Blink very confused when the drag comes to an eventual end.
> > 
> > A layout test that exercises non-crashing behavior will be added by
> > https://codereview.chromium.org/839253002/
> 
> Are you sure this crash can't be provoked in scenarios outside of event
sender? 
> Eg. maybe if I start dragging a file using touch drag and drop, and then do a
> mouse button down.  Or what if I'm dragging a file with one mouse device then
a
> second mouse device sends a left button down event?
> 
> If it's at all reasonable the fix the underlying issue with confusion in blink
> then I'd prefer that to an event_sender-specific fix.  It's nice to have
> clusterfuzz be able to generate arbitrary overlapped input through eventSender
> to simulate potential real-world scenarios.  But if we reasonably believe that
> such a scenario can never happen in practice (eg. maybe by a comment in event
> sender pointing at the code in chromium/blink that implements such a
restriction
> on real input events), then it's reasonable to add code like this to prevent
it
> in event_sender.

For this particular case, Blink treats the later mouse down as attempting to
initiate a new drag (the link being dragged over isn't dragged far enough for
that to really get started.) EventSender is oblivious to that drag restart (and
implicit cancellation of the initial file drag), so when it delivers the mouse
up that initial file drag is attempted "ended". Which Blink isn't prepared for.

We could address that by making Blink cope with such an unexpected/incorrect
drag end here, but it seems to me that the embedder should be coordinated about
what drag operation (and from what device) is under way. Does Blink really aim
to support multiple ongoing drag&drops, possibly from multiple devices? A page's
DragController doesn't appear to be tailored for that.

[Rick Byers, 2015-01-09 19:13:48]

On 2015/01/09 17:23:43, sof wrote:
> On 2015/01/09 17:01:08, Rick Byers wrote:
> > On 2015/01/09 16:50:30, sof wrote:
> > > Please take a look.
> > > 
> > > Allowing mouseDown() while a file drag has been initiated via eventSender,
> > just
> > > makes Blink very confused when the drag comes to an eventual end.
> > > 
> > > A layout test that exercises non-crashing behavior will be added by
> > > https://codereview.chromium.org/839253002/
> > 
> > Are you sure this crash can't be provoked in scenarios outside of event
> sender? 
> > Eg. maybe if I start dragging a file using touch drag and drop, and then do
a
> > mouse button down.  Or what if I'm dragging a file with one mouse device
then
> a
> > second mouse device sends a left button down event?
> > 
> > If it's at all reasonable the fix the underlying issue with confusion in
blink
> > then I'd prefer that to an event_sender-specific fix.  It's nice to have
> > clusterfuzz be able to generate arbitrary overlapped input through
eventSender
> > to simulate potential real-world scenarios.  But if we reasonably believe
that
> > such a scenario can never happen in practice (eg. maybe by a comment in
event
> > sender pointing at the code in chromium/blink that implements such a
> restriction
> > on real input events), then it's reasonable to add code like this to prevent
> it
> > in event_sender.
> 
> For this particular case, Blink treats the later mouse down as attempting to
> initiate a new drag (the link being dragged over isn't dragged far enough for
> that to really get started.) EventSender is oblivious to that drag restart
(and
> implicit cancellation of the initial file drag), so when it delivers the mouse
> up that initial file drag is attempted "ended". Which Blink isn't prepared
for.
> 
> We could address that by making Blink cope with such an unexpected/incorrect
> drag end here, but it seems to me that the embedder should be coordinated
about
> what drag operation (and from what device) is under way.

In general we try not to make blink rely entirely on the embedder to send
rational event streams.  There's just too many variables (including, eg. bugs
with drivers on Windows).  We also have a bunch of special case scenarios where
multiple types of input can compete (eg. during perf testing we send synthetic
input, but a user might touch the device at the same time).  So I'd like blink
to try to be robust to unusual event streams (although perhaps we should have a
'strict event mode' we can put it into to trigger ASSERTs on unexpected event
streams).
 
> Does Blink really aim
> to support multiple ongoing drag&drops, possibly from multiple devices? A
page's
> DragController doesn't appear to be tailored for that.

No, I don't think it does - but to me that's the bug, we don't handle that case
gracefully.  clusterfuzz/event_sender has done us a favor here by showing how
such an un-coordinated event stream can cause a crash.  We should try to fix the
crash, but I don't think we need to do anything smarter than just reject/ignore
the unexpected input (i.e. I don't really care how such a weird case behaves, as
long as the behavior doesn't cause problems like crashes, apparently stuck
input, etc.).

We also use event_sender to test blink fixes for crashes caused by unexpected
input event streams, so really it should ideally be capable of sending any
stream that the embedder can send (conceptually it's just a different embedder).

WDYT?

[sof, 2015-01-09 19:57:10]

On 2015/01/09 19:13:48, Rick Byers wrote:
> On 2015/01/09 17:23:43, sof wrote:
> > On 2015/01/09 17:01:08, Rick Byers wrote:
> > > On 2015/01/09 16:50:30, sof wrote:
> > > > Please take a look.
> > > > 
> > > > Allowing mouseDown() while a file drag has been initiated via
eventSender,
> > > just
> > > > makes Blink very confused when the drag comes to an eventual end.
> > > > 
> > > > A layout test that exercises non-crashing behavior will be added by
> > > > https://codereview.chromium.org/839253002/
> > > 
> > > Are you sure this crash can't be provoked in scenarios outside of event
> > sender? 
> > > Eg. maybe if I start dragging a file using touch drag and drop, and then
do
> a
> > > mouse button down.  Or what if I'm dragging a file with one mouse device
> then
> > a
> > > second mouse device sends a left button down event?
> > > 
> > > If it's at all reasonable the fix the underlying issue with confusion in
> blink
> > > then I'd prefer that to an event_sender-specific fix.  It's nice to have
> > > clusterfuzz be able to generate arbitrary overlapped input through
> eventSender
> > > to simulate potential real-world scenarios.  But if we reasonably believe
> that
> > > such a scenario can never happen in practice (eg. maybe by a comment in
> event
> > > sender pointing at the code in chromium/blink that implements such a
> > restriction
> > > on real input events), then it's reasonable to add code like this to
prevent
> > it
> > > in event_sender.
> > 
> > For this particular case, Blink treats the later mouse down as attempting to
> > initiate a new drag (the link being dragged over isn't dragged far enough
for
> > that to really get started.) EventSender is oblivious to that drag restart
> (and
> > implicit cancellation of the initial file drag), so when it delivers the
mouse
> > up that initial file drag is attempted "ended". Which Blink isn't prepared
> for.
> > 
> > We could address that by making Blink cope with such an unexpected/incorrect
> > drag end here, but it seems to me that the embedder should be coordinated
> about
> > what drag operation (and from what device) is under way.
> 
> In general we try not to make blink rely entirely on the embedder to send
> rational event streams.  There's just too many variables (including, eg. bugs
> with drivers on Windows).  We also have a bunch of special case scenarios
where
> multiple types of input can compete (eg. during perf testing we send synthetic
> input, but a user might touch the device at the same time).  So I'd like blink
> to try to be robust to unusual event streams (although perhaps we should have
a
> 'strict event mode' we can put it into to trigger ASSERTs on unexpected event
> streams).
>  
> > Does Blink really aim
> > to support multiple ongoing drag&drops, possibly from multiple devices? A
> page's
> > DragController doesn't appear to be tailored for that.
> 
> No, I don't think it does - but to me that's the bug, we don't handle that
case
> gracefully.  clusterfuzz/event_sender has done us a favor here by showing how
> such an un-coordinated event stream can cause a crash.  We should try to fix
the
> crash, but I don't think we need to do anything smarter than just
reject/ignore
> the unexpected input (i.e. I don't really care how such a weird case behaves,
as
> long as the behavior doesn't cause problems like crashes, apparently stuck
> input, etc.).
> 
> We also use event_sender to test blink fixes for crashes caused by unexpected
> input event streams, so really it should ideally be capable of sending any
> stream that the embedder can send (conceptually it's just a different
embedder).
> 
> WDYT?

I think you've convinced me that it'd be preferable to handle this on the Blink
side; will close this and continue at
https://codereview.chromium.org/839253002/. Hopefully you can be convinced to
come along as well? :-)