Don't refcount tracking id -> slot id mapping.

Don't refcount tracking id -> slot id mapping.

Previously we tried to refcount the tracking id to slot id mapping.
This broke in some circumstances where the number of press events was
not equal to the number of release events.

This patch switches to marking some touch events such that they don't
modify the mapping, simplifying logic, and fixing a nasty bug.

BUG=439051
TEST=EventsXTest.TouchEventNotRemovingFromNativeMapping

Committed: https://crrev.com/47a823b565a4051e58d83a857738c0fb8417d9ac
Cr-Commit-Position: refs/heads/master@{#313520}

[tdresser, 2014-12-06 17:58:25]

tdresser@chromium.org changed reviewers:
+ sadrul@chromium.org

[tdresser, 2014-12-06 17:58:25]

The approach I was thinking of using (checking if the event created a mapping
from a tracking id to a slot number) doesn't work (as you need the information
from the press event in the release event).

How does this look?

[tdresser, 2015-01-14 13:40:27]

On 2014/12/06 17:58:25, tdresser wrote:
> The approach I was thinking of using (checking if the event created a mapping
> from a tracking id to a slot number) doesn't work (as you need the information
> from the press event in the release event).
> 
> How does this look?

Ping.

[sadrul, 2015-01-14 20:21:35]

https://codereview.chromium.org/785753002/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/785753002/diff/20001/ui/events/event.cc#newco...
ui/events/event.cc:529: should_remove_native_touch_id_mapping_ = true;
Do this before or after the latency block.

https://codereview.chromium.org/785753002/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_event_dispatcher_linux.cc (right):

https://codereview.chromium.org/785753002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_event_dispatcher_linux.cc:81:
->set_should_remove_native_touch_id_mapping(false);
+ break;

Add a comment explaining why this is necessary.

Add a test for this in menu_controller_unittest.cc

[tdresser, 2015-01-23 18:10:59]

Patchset #4 (id:60001) has been deleted

[tdresser, 2015-01-23 18:12:34]

Sorry for the slow response here.

Can you take another look?

https://codereview.chromium.org/785753002/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/785753002/diff/20001/ui/events/event.cc#newco...
ui/events/event.cc:529: should_remove_native_touch_id_mapping_ = true;
On 2015/01/14 20:21:35, sadrul wrote:
> Do this before or after the latency block.

Done.

https://codereview.chromium.org/785753002/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_event_dispatcher_linux.cc (right):

https://codereview.chromium.org/785753002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_event_dispatcher_linux.cc:81:
->set_should_remove_native_touch_id_mapping(false);
On 2015/01/14 20:21:35, sadrul wrote:
> + break;
Done (eek, thanks)
> Add a comment explaining why this is necessary.
Done
> Add a test for this in menu_controller_unittest.cc
Done.

[sadrul, 2015-01-26 18:49:47]

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.cc#newco...
ui/events/event.cc:534: fixRotationAngle();
We should call this FixRotationAngle()
(https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h#newco...).
Mind fixing this up?

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:578: // event id and the chromium touch id. This should only
be the case
s/chromium touch id/touch_id_/

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:331: EXPECT_EQ(0,
test_event_handler.outstanding_touches());
Can you explain the expectation of the test here? (e.g. we are dispatching two
touch-begin events with different tracking ids, shouldn't we have 1 outstanding
touch?)

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:336: #endif
#endif  // defined(USE_X11)

[tdresser, 2015-01-27 21:43:06]

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.cc#newco...
ui/events/event.cc:534: fixRotationAngle();
On 2015/01/26 18:49:46, sadrul wrote:
> We should call this FixRotationAngle()
>
(https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h#newco...).
> Mind fixing this up?

Done.

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/785753002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:578: // event id and the chromium touch id. This should only
be the case
On 2015/01/26 18:49:46, sadrul wrote:
> s/chromium touch id/touch_id_/

Done.

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:331: EXPECT_EQ(0,
test_event_handler.outstanding_touches());
On 2015/01/26 18:49:46, sadrul wrote:
> Can you explain the expectation of the test here? (e.g. we are dispatching two
> touch-begin events with different tracking ids, shouldn't we have 1
outstanding
> touch?)

We dispatch 2 touch begins, and two touch ends (one of them happens in a
PostTask.)

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:336: #endif
On 2015/01/26 18:49:46, sadrul wrote:
> #endif  // defined(USE_X11)

Done.

[sadrul, 2015-01-28 00:04:05]

LGTM

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/785753002/diff/80001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:331: EXPECT_EQ(0,
test_event_handler.outstanding_touches());
On 2015/01/27 21:43:05, tdresser wrote:
> On 2015/01/26 18:49:46, sadrul wrote:
> > Can you explain the expectation of the test here? (e.g. we are dispatching
two
> > touch-begin events with different tracking ids, shouldn't we have 1
> outstanding
> > touch?)
> 
> We dispatch 2 touch begins, and two touch ends (one of them happens in a
> PostTask.)

Oh whoops. Missed that! Sorry

https://codereview.chromium.org/785753002/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/785753002/diff/100001/ui/events/event.h#newco...
ui/events/event.h:581: // id.
s/chromium touch id/|touch_id_|/

[tdresser, 2015-01-28 14:52:14]

https://codereview.chromium.org/785753002/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/785753002/diff/100001/ui/events/event.h#newco...
ui/events/event.h:581: // id.
On 2015/01/28 00:04:05, sadrul wrote:
> s/chromium touch id/|touch_id_|/

Done.

[tdresser, 2015-01-28 14:52:18]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2015-01-28 14:53:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/785753002/120001

[commit-bot: I haz the power, 2015-01-28 15:53:40]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-01-28 15:54:46]

Patchset 6 (id:??) landed as
https://crrev.com/47a823b565a4051e58d83a857738c0fb8417d9ac
Cr-Commit-Position: refs/heads/master@{#313520}

[tdresser, 2015-01-29 13:56:29]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/869433007/ by tdresser@chromium.org.

The reason for reverting is: Reverting due to memory failures.

http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...

{
UNINITIALIZED READ
name=<insert_a_suppression_name_here>
content.dll!content::TouchEventQueue::TouchMoveSlopSuppressor::FilterEvent
content.dll!content::TouchEventQueue::FilterBeforeForwarding
content.dll!content::TouchEventQueue::TryForwardNextEventToRenderer
content.dll!content::TouchEventQueue::ProcessTouchAck
content.dll!content::InputRouterImpl::ProcessInputEventAck
content.dll!content::InputRouterImpl::OnInputEventAck
content.dll!InputHostMsg_HandleInputEvent_ACK::Dispatch<>
content.dll!content::InputRouterImpl::OnMessageReceived
content.dll!content::RenderWidgetHostImpl::OnMessageReceived
*!content::RenderWidgetHostViewAuraTest_TouchEventState_Test::TestBody
*!testing::internal::HandleExceptionsInMethodIfSupported<>
}

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...

UninitCondition
Conditional jump or move depends on uninitialised value(s)
 
content::TouchEventQueue::TouchMoveSlopSuppressor::FilterEvent(blink::WebTouchEvent
const&) (content/browser/renderer_host/input/touch_event_queue.cc:240)
  content::TouchEventQueue::FilterBeforeForwarding(blink::WebTouchEvent const&)
(content/browser/renderer_host/input/touch_event_queue.cc:715)
  content::TouchEventQueue::TryForwardNextEventToRenderer()
(content/browser/renderer_host/input/touch_event_queue.cc:445)
  content::TouchEventQueue::ProcessTouchAck(content::InputEventAckState,
ui::LatencyInfo const&)
(content/browser/renderer_host/input/touch_event_queue.cc:436)
  content::InputRouterImpl::ProcessTouchAck(content::InputEventAckState,
ui::LatencyInfo const&)
(content/browser/renderer_host/input/input_router_impl.cc:666)
  content::InputRouterImpl::ProcessInputEventAck(blink::WebInputEvent::Type,
content::InputEventAckState, ui::LatencyInfo const&,
content::InputRouterImpl::AckSource)
(content/browser/renderer_host/input/input_router_impl.cc:576)
 
content::InputRouterImpl::OnInputEventAck(InputHostMsg_HandleInputEvent_ACK_Params
const&) (content/browser/renderer_host/input/input_router_impl.cc:483)
 
_Z20DispatchToMethodImplIN7content15InputRouterImplEMS1_FvRK40InputHostMsg_HandleInputEvent_ACK_ParamsEJS2_EJLm0EEEvPT_T0_RK5TupleIJDpT1_EE13IndexSequenceIJXspT2_EEE
(base/tuple.h:246)
Suppression (error hash=#675E7177BFE2BE12#):
  For more info on using suppressions see
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
{
   <insert_a_suppression_name_here>
   Memcheck:Uninitialized
  
fun:_ZN7content15TouchEventQueue23TouchMoveSlopSuppressor11FilterEventERKN5blink13WebTouchEventE
  
fun:_ZN7content15TouchEventQueue22FilterBeforeForwardingERKN5blink13WebTouchEventE
   fun:_ZN7content15TouchEventQueue29TryForwardNextEventToRendererEv
  
fun:_ZN7content15TouchEventQueue15ProcessTouchAckENS_18InputEventAckStateERKN2ui11LatencyInfoE
  
fun:_ZN7content15InputRouterImpl15ProcessTouchAckENS_18InputEventAckStateERKN2ui11LatencyInfoE
  
fun:_ZN7content15InputRouterImpl20ProcessInputEventAckEN5blink13WebInputEvent4TypeENS_18InputEventAckStateERKN2ui11LatencyInfoENS0_9AckSourceE
  
fun:_ZN7content15InputRouterImpl15OnInputEventAckERK40InputHostMsg_HandleInputEvent_ACK_Params
  
fun:_Z20DispatchToMethodImplIN7content15InputRouterImplEMS1_FvRK40InputHostMsg_HandleInputEvent_ACK_ParamsEJS2_EJLm0EEEvPT_T0_RK5TupleIJDpT1_EE13IndexSequenceIJXspT2_EEE
}

I haven't been able to reproduce this consistently locally, which is making it
tricky to pin down. Once I have (or I've determined this patch isn't what's
causing the failures), I'll reland..

[jdduke (slow), 2015-02-03 00:24:56]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-02-03 00:24:57]

https://codereview.chromium.org/785753002/diff/120001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/785753002/diff/120001/ui/events/event.h#newco...
ui/events/event.h:495: should_remove_native_touch_id_mapping_(false) {}
Looks like a bad rebase. |may_cause_scrolling_| is no longer initialized,
neither here nor in the other constructors.