TouchEventQueue needs to untangle a cyclic calling sequence |
||||||
Issue descriptionIt doesn't make sense that TouchEventQueue::SendTouchEventImmediately() would call itself through a cycling calling sequence. But unfortunately a crack in logic somewhere is causing this to happen. We should fix this by untangling the design, rather than adding logic to avoid reentry. When a touchend input is received while there is a pending async touchmove, the following sequence of calls seems to take place: 1. QueueEvent() gets called for the touchend input. 2. ForwardNextEventToRenderer(). 3. FlushPendingAsyncTouchmove() because there was a pending async touchmove, which now gets prepended to the queue. 4. SendTouchEventImmediately() to dispatch the prepended touchmove. 5. TryForwardNextEventToRenderer() 6. ForwardNextEventToRenderer() 7. SendTouchEventImmediately() to dispatch the touchmove. This effectively results in two in-flight events in the queue. This calling sequence seems easily reproducible, but this doesn't cause any failure only because of "lucky timing" of the acks from renderer I believe. We were hit by the timing issue when working on this patch: https://codereview.chromium.org/1800143002/#ps200001.
,
Apr 5 2016
This is likely because the TouchEnd is not cancelable (can you confirm?). If so then the touch end is ack'd by the synthetic event generated by the input_router (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/input/input_router_impl.cc&sq=package:chromium&type=cs&l=388&rcl=1459854954) And the touchmove actually is ack'd by the main thread.
,
Apr 5 2016
See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/input/touch_event_queue.cc&rcl=1459854954&l=589 for why SendTouchEventImmediately is re-entrant. It seems like FlushPendingAsyncTouchmove shouldn't call SendTouchEventImmediately, but should call TryForwardNextEventToRenderer, though I'm not sure that's the whole problem. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/input/touch_event_queue.cc&rcl=1459854954&l=630
,
Apr 5 2016
@Dave: yes, the touchend ack arrives the touchmove ack only when it is uncancelable. @Tim: Calling TryForwardNextEventToRenderer instead won't solve the problem because ultimately it would reach ForwardNextEventToRenderer which doesn't expect a reentry (DCHECKs dispatching_touch_ == false). May be that's why the current code bypasses (Try)ForwardNextEventToRenderer. @both: the cases in #2 and #3 are somewhat independent but both seem to suggest that we can't rely on relative ordering of the acks. In fact we are not relying much, just a DCHECK is there. I will remove the remove the DCHECK for now as a tentative solution. Sg?
,
Apr 5 2016
For #2 above, the touchmove is also uncancelable. So it does make sense to wait for the (synthetic) ack before sending the touchend.
,
Apr 5 2016
If we call TryForwardNextEventToRenderer while dispatching_touch_ is true, shouldn't we just return early?
,
Apr 5 2016
Makes sense. I think we would still want to send the upstream acks (PopTouchEventToClient), right? If this is correct, I think we can bail out from ForwardNextEventToRenderer instead, upon reentry.
,
Apr 6 2016
We wouldn't need to PopTouchEventToClient any differently than we normally do, would we?
,
Apr 6 2016
While it was easy to prevent reentry to SendTouchEventImmediately, I was still able to get out-of-order acks: new incoming touch events can still trigger the dispatch of the uncancelable touchend right after the last touchmove (before its ack arrives), then the (local synthetic) ack for the touchend is received before the touchmove ack. The ordering assumption is clearly wrong. I will remove the DCHECK as I suggested in #4.
,
Apr 6 2016
We shouldn't send the uncancelable touchend before the previous async touchmove is acked, should we? It's just waiting on a synthetic ack.
,
Apr 8 2016
You are right, but somehow this is happening even after preventing reentry. I am adding a hack in crrev.com/1800143002 for now to prevent reentry. Will focus on the untangling code question later on.
,
Apr 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b3c44a0beb7e5a56fc78861c22b588380de3f49b commit b3c44a0beb7e5a56fc78861c22b588380de3f49b Author: mustaq <mustaq@chromium.org> Date: Tue Apr 19 18:49:05 2016 Notify Blink about start of gesture scroll through a queued event. Added WebIputEventL::TouchScrollStarted event to notify Blink when a gesture scroll is starting. We need to know from Blink when gesture scroll is about to start. So far, the only use of this info is firing a pointercancel event during scrolling. We had been using a bit in WebTouchEvent before, which didn't really reflect the possibility of scrolling (only indicates if the touch has moved beyond the slop region). To find the correct info to pass to Blink, we have to consider the effect of touch-action on gestures. In this CL, we push a new event into the touch queue when gestures passes touch-action filtering. This CL also adds a hack to prevent unintended reentry to a dispatch method in TouchEventQueue. BUG= 567740 , 600773 Review URL: https://codereview.chromium.org/1800143002 Cr-Commit-Position: refs/heads/master@{#388258} [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/blimp/net/input_message_generator.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/components/test_runner/event_sender.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/components/test_runner/event_sender.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/input/input_router_impl_unittest.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/input/touch_event_queue.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/input/touch_event_queue.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/input/touch_event_queue_unittest.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/render_widget_host_unittest.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/common/input/touch_event_stream_validator.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/common/input/web_input_event_traits.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/common/input/web_touch_event_traits.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/content/renderer/input/render_widget_input_handler.cc [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel-expected.txt [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-pointercancel-expected.txt [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/events/PointerEventFactory.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/events/PointerEventFactory.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/input/EventHandler.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/input/EventHandler.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/input/PointerEventManager.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/core/input/PointerEventManager.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/platform/PlatformEvent.h [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/web/PageWidgetDelegate.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/Source/web/WebInputEventConversion.cpp [modify] https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b/third_party/WebKit/public/web/WebInputEvent.h
,
Feb 16 2017
,
Mar 15 2017
This bug is about an old code (old touch_event_queue.cc renamed to legacy_touch_event_queue.cc few months ago). No longer applicable.
,
Apr 13 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mustaq@chromium.org
, Apr 5 2016