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

Fixed NPE in LoginUsernamePasswordFragment #10833

Merged
merged 2 commits into from Dec 11, 2019

Conversation

@khaykov
Copy link
Member

khaykov commented Nov 22, 2019

Fixes #10832

After the user leaves LoginUsernamePasswordFragment for more than one hour and comes back, a global site refresh is triggered, which causes onSiteChanged in LoginUsernamePasswordFragment to propagate false "successful" login event, which causes a crash down the line.

To make the testing easier, change the throttle delay on-site refresh (here) to something reasonable, like 10 seconds.

To test:

  • Navigate to Site Picker and press the + icon to add new site.
  • Chose "Add self-hosted site" option.
  • In the Site address field input self-hosted site URL and press NEXT.
  • While on the screen with login and password, press the home button and minimize the app.
  • Wait 10 seconds (or whatever time you chouse for site fetch throttle.
  • Navigate back to the app, and notice that it's not crashing.

Part 2:

  • Input wrong login or password and press NEXT to produce an error.
  • Press the home button and minimize the app.
  • Navigate back to the app and input correct login credentials.
  • Make sure you can login and the site is added.

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@khaykov khaykov added this to the 13.8 milestone Nov 22, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 22, 2019

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout issue/10832-fix-npe-in-login-fragment
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/10833
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/10833 and open a new PR.

Generated by 🚫 dangerJS

@khaykov khaykov changed the title Added flag that checks if login has started. Fixed NPE in LoginUsernamePasswordFragment Nov 22, 2019
@khaykov khaykov requested review from planarvoid, develric and malinajirka Nov 22, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 22, 2019

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka malinajirka self-assigned this Nov 23, 2019
Copy link
Contributor

malinajirka left a comment

Great investigation @khaykov !! I've tried to carefully review and test the code and AFAICT it works like a charm. However, tbh it's quite complicated and since it's a vital part of the app, I'd feel more comfortable if one more developer reviewed it.
cc @planarvoid @develric

I'd also suggest keeping an eye on "failed login" tracks to make sure we didn't break anything.

@malinajirka malinajirka removed their assignment Nov 23, 2019
@@ -253,6 +256,7 @@ public void onSaveInstanceState(Bundle outState) {
super.onSaveInstanceState(outState);

outState.putBoolean(KEY_LOGIN_FINISHED, mLoginFinished);
outState.putBoolean(KEY_LOGIN_FINISHED, mLoginStarted);

This comment has been minimized.

Copy link
@planarvoid

planarvoid Nov 25, 2019

Contributor

I think the key here is wrong, it should be KEY_LOGIN_STARTED

This comment has been minimized.

Copy link
@malinajirka

malinajirka Nov 25, 2019

Contributor

:D How come I missed this. Great catch !

@jkmassel

This comment has been minimized.

Copy link
Contributor

jkmassel commented Dec 3, 2019

We're freezing 13.8 today, so I'm moving this forward to 13.9. If you'd like it released as part of 13.8, please adjust the target from develop to release/13.8, and let me know once this PR has been merged – I'll be happy to ship a new release candidate!

@jkmassel jkmassel modified the milestones: 13.8, 13.9 Dec 3, 2019
Copy link
Contributor

planarvoid left a comment

There is one mistake when saving the state 👍

@khaykov

This comment has been minimized.

Copy link
Member Author

khaykov commented Dec 11, 2019

I fixed the issue, so this one is ready for another round :)

Copy link
Contributor

planarvoid left a comment

thanks for the change 👍, it works well :shipit:

@planarvoid planarvoid merged commit e2e6e29 into develop Dec 11, 2019
6 checks passed
6 checks passed
Peril All green. Nice work.
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: connected-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@planarvoid planarvoid deleted the issue/10832-fix-npe-in-login-fragment branch Dec 11, 2019
@sentry-io

This comment has been minimized.

Copy link

sentry-io bot commented Dec 19, 2019

Sentry issue: WORDPRESS-ANDROID-3C

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.