wordpress-mobile / WordPress-Login-Flow-Android

Pluggable WordPress login flow for Android
GNU General Public License v2.0
14 stars 3 forks source link

Adds non-null check to onActivityResult #16

Closed planarvoid closed 4 years ago

planarvoid commented 5 years ago

Fixes - https://github.com/wordpress-mobile/WordPress-Android/issues/7362 I'm pretty sure this NPE happens when the view gets destroyed right after onActivityResult happens. I'm adding a non-null check in the postDelayed method so we don't show a keyboard if the view is already gone. I know that the isAdded check is there for the same reason but it seems to not work correctly.

aforcier commented 5 years ago

@planarvoid I'm not sure whether this will fix the issue. From 5bec9095f8b88c2963f23d27-fabric and looking at LoginEmailFragment for WPAndroid 12.0, it looks like the crash is happening a little further up:

https://github.com/wordpress-mobile/WordPress-Android/blob/ad1e7f4be754823fbad7453763650130b730ddbb/libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginEmailFragment.java#L454

It's weird since we null check further up, I suppose something has changed by the time the execution reaches that line in this case.

planarvoid commented 5 years ago

@aforcier Ah you're right, the crash I saw actually happens in the SignupEmailFragment so the fix should be there. I think we've mixed 2 independent crashes with very similar causes. I was actually confused that the line numbers don't match the code I saw. I thought that it has changed already and this felt like the only place that could possible crash. Anyway - everything is correct now and this is the crash that's being fixed - https://sentry.io/organizations/a8c/issues/1019195395/?project=1438088&referrer=github_integration

aforcier commented 4 years ago

Ah okay @planarvoid I think this makes sense. It looks like the same issue was fixed for LoginEmailFragment in https://github.com/woocommerce/woocommerce-android/pull/413, and the crash stopped happening after WPAndroid 12.2, which was around when that change made it through the login library and into WPAndroid.

Change looks good, :shipit: