Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race conditions for Android Support SDK 27 #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kristfal
Copy link

@kristfal kristfal commented Jul 23, 2018

We'll deploy to production this Wednesday. I propose you wait for us to get analytics from users before we merge. Fixes #42.

@kristfal
Copy link
Author

So the update is live, and we're still seeing crashes, but at a new location in the function. @cesardeazevedo, this is the new crash stack:

java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.Object java.lang.ref.WeakReference.get()' on a null object reference

com.bottomsheetbehavior.RNBottomSheetBehavior.onInterceptTouchEvent RNBottomSheetBehavior.java:267

android.support.design.widget.CoordinatorLayout.resetTouchBehaviors CoordinatorLayout.java:394
android.support.design.widget.CoordinatorLayout.onInterceptTouchEvent CoordinatorLayout.java:509
android.view.ViewGroup.dispatchTouchEvent ViewGroup.java:2504

Which is here.

I'm wondering if we should do something like:

if ( mViewDragHelper == null ) {
      return false;
}

At the top of the function instead to exit early instead. Do you think this will have any negative side effects beyond "ignoring" the first touch before everything is set up properly?

@cesardeazevedo
Copy link
Owner

Thanks for the report, i am going to take a look more closely this weekend, i am wondering if a full rebase of the BottomSheetBehavior from the SDK 27 is needed, since i have implemented the anchor point feature, the source had to be copied, and was likely copied from the SDK 25 instead.

@kristfal
Copy link
Author

Thanks, that may be a solution. In the short term, do you see any clear issues with returning false? We’re aiming to get a fix out to prod as soon as possible.

@cesardeazevedo
Copy link
Owner

I am not sure about returning false, i need more tests before give you such claim, the hardest part is to get a way to reproduce the problem, if you could help me with it, i would really appreciate.

@kristfal
Copy link
Author

The race condition happens very rarely (some 120 crashes per week amongst 30k active units with this component), so its very hard to tell what happens and how. I'm guessing there may be a touch event triggered during init of the component, but its hard to tell.

We'll try returning false as an interim stopgap regardless. I'll let you know how it works out.

@cesardeazevedo
Copy link
Owner

I am really happy to know that this component has been used quite often, let me know about the results, and if there's anything that we can do to overcome it.

@dmkmedina
Copy link

Hey @kristfal,

I'm curious if adding the null check for mViewDragHelper helped resolve the issue for you. We're facing similar issues on our end.

@kristfal
Copy link
Author

@dmkmedina I don't remember the specifics, but I am afraid the Java solution wasn't adequate. We ended up deferring loading TouchThroughWrapper until later in the app's lifecycle from the JS side. Haven't seen any crashes since.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null pointer exception shouldInterceptTouchEvent
3 participants