Skip to content

Commit

Permalink
Don't use MotionEvent timestamp for touch events
Browse files Browse the repository at this point in the history 
Differential Revision: D2554905

fb-gh-sync-id: 7f83e94948cc9ac1024e249764d445fb056f400e
  • Loading branch information
@andreicoman11
andreicoman11 authored and facebook-github-bot-7 committed Oct 19, 2015
1 parent e3f33ea commit 716f7e8
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 8 deletions.
22 changes: 16 additions & 6 deletions ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import android.content.Context;
import android.graphics.Rect;
import android.os.Bundle;
import android.os.SystemClock;
import android.util.AttributeSet;
import android.view.MotionEvent;
import android.view.View;
Expand Down Expand Up @@ -143,7 +144,8 @@ private void handleTouchEvent(MotionEvent ev) {
// this gesture
mChildIsHandlingNativeGesture = false;
mTargetTag = TouchTargetHelper.findTargetTagForTouch(ev.getY(), ev.getX(), this);
eventDispatcher.dispatchEvent(new TouchEvent(mTargetTag, TouchEventType.START, ev));
eventDispatcher.dispatchEvent(
new TouchEvent(mTargetTag, SystemClock.uptimeMillis(),TouchEventType.START, ev));
} else if (mChildIsHandlingNativeGesture) {
// If the touch was intercepted by a child, we've already sent a cancel event to JS for this
// gesture, so we shouldn't send any more touches related to it.
Expand All @@ -158,17 +160,21 @@ private void handleTouchEvent(MotionEvent ev) {
} else if (action == MotionEvent.ACTION_UP) {
// End of the gesture. We reset target tag to -1 and expect no further event associated with
// this gesture.
eventDispatcher.dispatchEvent(new TouchEvent(mTargetTag, TouchEventType.END, ev));
eventDispatcher.dispatchEvent(
new TouchEvent(mTargetTag, SystemClock.uptimeMillis(), TouchEventType.END, ev));
mTargetTag = -1;
} else if (action == MotionEvent.ACTION_MOVE) {
// Update pointer position for current gesture
eventDispatcher.dispatchEvent(new TouchEvent(mTargetTag, TouchEventType.MOVE, ev));
eventDispatcher.dispatchEvent(
new TouchEvent(mTargetTag, SystemClock.uptimeMillis(), TouchEventType.MOVE, ev));
} else if (action == MotionEvent.ACTION_POINTER_DOWN) {
// New pointer goes down, this can only happen after ACTION_DOWN is sent for the first pointer
eventDispatcher.dispatchEvent(new TouchEvent(mTargetTag, TouchEventType.START, ev));
eventDispatcher.dispatchEvent(
new TouchEvent(mTargetTag, SystemClock.uptimeMillis(), TouchEventType.START, ev));
} else if (action == MotionEvent.ACTION_POINTER_UP) {
// Exactly onw of the pointers goes up
eventDispatcher.dispatchEvent(new TouchEvent(mTargetTag, TouchEventType.END, ev));
eventDispatcher.dispatchEvent(
new TouchEvent(mTargetTag, SystemClock.uptimeMillis(), TouchEventType.END, ev));
} else if (action == MotionEvent.ACTION_CANCEL) {
dispatchCancelEvent(ev);
mTargetTag = -1;
Expand Down Expand Up @@ -213,7 +219,11 @@ private void dispatchCancelEvent(MotionEvent androidEvent) {
!mChildIsHandlingNativeGesture,
"Expected to not have already sent a cancel for this gesture");
Assertions.assertNotNull(eventDispatcher).dispatchEvent(
new TouchEvent(mTargetTag, TouchEventType.CANCEL, androidEvent));
new TouchEvent(
mTargetTag,
SystemClock.uptimeMillis(),
TouchEventType.CANCEL,
androidEvent));
}

@Override
Expand Down
8 changes: 6 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ public class TouchEvent extends Event<TouchEvent> {
private final TouchEventType mTouchEventType;
private final short mCoalescingKey;

public TouchEvent(int viewTag, TouchEventType touchEventType, MotionEvent motionEventToCopy) {
super(viewTag, motionEventToCopy.getEventTime());
public TouchEvent(
int viewTag,
long timestampMs,
TouchEventType touchEventType,
MotionEvent motionEventToCopy) {
super(viewTag, timestampMs);
mTouchEventType = touchEventType;
mMotionEvent = MotionEvent.obtain(motionEventToCopy);

Expand Down

1 comment on commit 716f7e8

@andreicoman11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some background on this diff:
Since v1, the highest firing crash in ads manager android was related to JS processing touch end events before touch start. This diff finally fixes that.

TLDR: android was supplying timestamps for touch end events that came before touch start events. ReactNative sorted these events by their timestamps, so JS got the touch end event before the touch start event of the same touch event, and crashed. The fix: generate our own timestamp when dispatching touch events.

Long dramatic story:

  • Initially we weren't sorting the touch events at all, so we thought that was the cause of this crash, fixed it and called it a day.
  • The fix only made the problem worse, and nobody knew why.
  • We found some more inaccuracies in our event dispatcher and in some other components that might have caused this crash, so we fixed those and hoped that that would somehow fix this bug.
  • The crash rate didn't change at all. What made matters worse, was that users started to report that madman was crashing 'after 2 or 3 clicks' or 'tapping the menu bar'. Nobody managed to repro any of this.
  • Since we had no idea what was going on, we added whatever debug information we could get our hands on, force crash the app as soon as we see this problem, and ship that in a RC.
  • So we did, and we started to see that indeed from the native side we were sending a touch end event that had no touch start. WAT.
  • We added more debugging information and shipped that to the next RC. Now we saw that for each touch end event that would crash the app, there was a touch start event sitting next in the dispatch queue. And these touch events were ordered correctly because the timestamp for touch end was smaller than the one for touch start. WAT.
  • The timestamps we were using for touch events were all coming from MotionEvent#getEventTime() supplied by android, since we thought those were the most reliable timestamps we could get. So we changed them to use the dispatch timestamp of the events instead. The problem immediately disappeared.

Lessons learned: question everything, especially android.

Please sign in to comment.