[Android Crash] Touch data should have been recorded on start #5246
Comments
|
Hey Richard-Cao, thanks for reporting this issue! React Native, as you've probably heard, is getting really popular and truth is we're getting a bit overwhelmed by the activity surrounding it. There are just too many issues for us to manage properly.
|
|
Sorry. I've no context on this. What do you mean by the touch gestures feature? How to enable it? |
|
@satya164 Some Android devices like XiaoMi, Huawei, Nubia and so on, have |
|
I just got this on iOS as well. It seems to happen when I try to scroll my ListView quickly after it mounts... |
|
Also got this on iOS when randomly tapping around on an app. Can't reproduce reliably ... so, sorry for there not being any more context. I don't think it was in the context of a ListView. |
|
Found where this is happening: On a cold boot of my app, if the first touch on the screen is a scroll on a ListView/ScrollView, there appears to be a race condition and Upon the first call, if (touchTrack) {
reinitializeTouchTrack(touchTrack, touch);
} else {
touchBank[touch.identifier] = initializeTouchData(touch);
}(like it exists in |
|
Opened an issue on react core here facebook/react#6277 |
|
Ok, they bounced it back over here. Anyone familiar enough with |
|
@majak I know you recently refactored how touches work (by passing them through the event dispatcher)...would you have any clue on this issue? TLDR; we're getting touchesMoved before a touchesStart is ever sent. |
|
(cc @nicklockwood) |
|
Yep, you can see here iOS gets This is on React Native 0.20 |
|
I've changed how touches are handled only on iOS side. If this issue is common for both iOS and Android I would expect something is wrong on js side. |
|
@majak I'll try to throw something together. The tricky thing is it's not reproducible 100% of the time (definitely a race condition). But anyway, yeah let me see if I can get something up and running. |
|
@majak Marc and I did a bit more research on this: Here, we're logging calls to the bridge with the method "receiveTouches", referring to the method that receives touches in OUTSIDE BLOCK: https://github.com/facebook/react-native/blob/master/React/Base/RCTBatchedBridge.m#L696 As you can see in the log, the "OUTSIDE_BLOCK" stuff is executing in the proper order -- a topTouchStart followed by a topTouchMove. INSIDE_BLOCK, however, is executing in the wrong order. You can see the topTouchStart event doesn't ACTUALLY get executed on the bridge until after the the topTouchMove. This seems weird, but I think I may have a fundamental misunderstanding of how the JSC executor works. I was under the impression that while timing of a given JS call couldn't be guaranteed (due to the async nature of the bridge), the order could be, because as the method name implies, updates are "enqueued". Obviously this could be fixed in JS by queuing up move calls until a touchStart is received, and we can enforce the order there. Perhaps the right fix, and presumably that would then be fixed on both platforms. I'm curious though if anyone can explain the executor here though, and why order can't be guaranteed. CCing @javache here as well. |
|
Great data! I've chatted with @tadeuzagallo and we figured out there is indeed a race condition in how touches are processed. For (perf) reasons,
I'll fix it. Thanks for digging into this, @skevy & @marcshilling! This also makes me think this github issue actually contains two different bugs that surface with the same redbox. |
|
This is a great explanation, and makes a lot of sense conceptually. I'm not sure I'm clear where exactly in the code this is happening...but I'll wait for the diff! Thanks so much @majak and @tadeuzagallo. |
|
Seeing similar crash reports in the wild on iOS as well. Thanks for looking into it! |
Summary:Previously, (mostly touch and scroll) event handling on iOS worked in a hybrid way: * All incoming coalesce-able events would be pooled and retrieved by js thread in the beginning of its frame (all of this happens on js thread) * Any non-coalesce-able event would be immediately dispatched on a js thread (triggered from main thread), and if there would be pooled coalesce-able events they would be immediately dispatched at first too. This behavior has a subtle race condition, where two events are produced (on MT) in one order and received in js in different order. See #5246 (comment) for further explanation of this case. The new event handling is (afaik) what Android already does. When an event comes we add it into a pool of events and dispatch a block on js thread to inform js there are events to be processed. We keep track of whether we did so, so there is at most one of these blocks waiting to be processed. When the block is executed js will process all events that are in pool at that time (NOT at time of enqueuing the block). This creates a single way of processing events and makes it impossible to process them in different order in js. The tricky part was making sure we don't coalesce events across gestures/different scrolls. Before this was achieved by knowing that gestures and scrolls start/end with non-coalesce-able event, so the pool never contained events that shouldn't be coalesced together. That "assumption" doesn't hold now. I've re-added `coalescingKey` and made touch and scroll events use it to prevent coalescing events of the same type that should remain separate in previous diffs (see dependencies). On top of it it decreases latency in events processing in case where we get only coalesce-able events. Previously these would be processed at begging of the next js frame, even when js would be free and could process them sooner. This delay is done, since they would get processed as soon as the enqueued block would run. To illustrate this improvement let's look at these two systraces. Before: https://cloud.githubusercontent.com/assets/713625/14021417/47b35b7a-f1d3-11e5-93dd-4363edfa1923.png After: https://cloud.githubusercontent.com/assets/713625/14021415/4798582a-f1d3-11e5-8715-425596e0781c.png Reviewed By: javache Differential Revision: D3092867 fb-gh-sync-id: 29071780f00fcddb0b1886a46caabdb3da1d5d84 fbshipit-source-id: 29071780f00fcddb0b1886a46caabdb3da1d5d84
|
Changes that should fix this issue were just pushed on master. Commits from 8f07b01 to 31bb85a. The important one with deeper explanation is 1d3db4c. But also turn off chrome debugging please. Looks like this stack breaks it 😅. I'm looking into that too. |
|
Version 0.24.0-rc1 should already contain the fix and all necessary fixes for the fix. |
|
To be clear, we believe the issue on iOS has been fixed. Those commits don't affect Android. |
Summary:Previously, (mostly touch and scroll) event handling on iOS worked in a hybrid way: * All incoming coalesce-able events would be pooled and retrieved by js thread in the beginning of its frame (all of this happens on js thread) * Any non-coalesce-able event would be immediately dispatched on a js thread (triggered from main thread), and if there would be pooled coalesce-able events they would be immediately dispatched at first too. This behavior has a subtle race condition, where two events are produced (on MT) in one order and received in js in different order. See facebook#5246 (comment) for further explanation of this case. The new event handling is (afaik) what Android already does. When an event comes we add it into a pool of events and dispatch a block on js thread to inform js there are events to be processed. We keep track of whether we did so, so there is at most one of these blocks waiting to be processed. When the block is executed js will process all events that are in pool at that time (NOT at time of enqueuing the block). This creates a single way of processing events and makes it impossible to process them in different order in js. The tricky part was making sure we don't coalesce events across gestures/different scrolls. Before this was achieved by knowing that gestures and scrolls start/end with non-coalesce-able event, so the pool never contained events that shouldn't be coalesced together. That "assumption" doesn't hold now. I've re-added `coalescingKey` and made touch and scroll events use it to prevent coalescing events of the same type that should remain separate in previous diffs (see dependencies). On top of it it decreases latency in events processing in case where we get only coalesce-able events. Previously these would be processed at begging of the next js frame, even when js would be free and could process them sooner. This delay is done, since they would get processed as soon as the enqueued block would run. To illustrate this improvement let's look at these two systraces. Before: https://cloud.githubusercontent.com/assets/713625/14021417/47b35b7a-f1d3-11e5-93dd-4363edfa1923.png After: https://cloud.githubusercontent.com/assets/713625/14021415/4798582a-f1d3-11e5-8715-425596e0781c.png Reviewed By: javache Differential Revision: D3092867 fb-gh-sync-id: 29071780f00fcddb0b1886a46caabdb3da1d5d84 fbshipit-source-id: 29071780f00fcddb0b1886a46caabdb3da1d5d84
|
Does anyone know whether this is still an issue on Android? It seems like we at least half fixed this type of problem, but since it has been a while it does not seem clear to me whether there is still a problem on Android here.. |
|
FWIW, I just saw this in Production on Android v6.0.1 with React Native v0.25.1 (which was tagged on May 4th). |
|
This is still reproducible on Android, RN v0.30 |
|
Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally! If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:
If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution. |
Paltform: Android
Version: 5.0
If you open
Touch gesturesfeature on your device, when three fingers on the screen at the same time, crash.There is crash stack:
I test this on react-native showcase
Movie Trailers by MovieLaLaAndroid, crash still exist.The text was updated successfully, but these errors were encountered: