Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616643005:
[SignIn] Fix AccountSigninView for asynchronous updateAccounts() (Closed)
Description[SignIn] Fix AccountSigninView for asynchronous updateAccounts()
updateAccounts() now takes a callback to allow code to run once the
accounts have actually been fetched.
This also adds some check in updateAccounts() to ensure it only updates
the accounts when expected (i.e. when the user isn't signed in).
BUG=678543, 672255
Review-Url: https://codereview.chromium.org/2616643005
Cr-Commit-Position: refs/heads/master@{#442932}
Committed: https://chromium.googlesource.com/chromium/src/+/1bfabdd7fe941a66ae44746e0fed8fd34ad61302
Patch Set 1 #
Total comments: 20
Patch Set 2 : Better comments, remove unecessary GMS check #Patch Set 3 : Comment early return #Messages
Total messages: 18 (7 generated)
bzanotti@chromium.org changed reviewers: + bauerb@chromium.org
Please take a look. Note that this fixes the crash from http://crbug.com/672255 (I'll repurpose the bug so we remember to add the long-term fix to AccountManagerHelper.getGoogleAccountNames).
bzanotti@chromium.org changed reviewers: + gogerald@chromium.org - bauerb@chromium.org
Switching to a more local (and not OOO) OWNERS. Ganggui: Please take a look.
https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:240: * @param callback Called once the accounts have been refreshed. Boolean Make it: /** * Refresh the list of available system accounts asynchronously. * * @param callback Called once the accounts have been refreshed. Boolean * indicates whether something went wrong. */ https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:251: callback.onResult(false); Thinking one situation: FRE forced sign in will return here if GMS is out of date (only one account on device). When user come back to Chrome after updating GMS (do not kill Chrome), the updateAccounts() is called without showing confirmation screen directly. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:497: if (!checkGooglePlayServicesAvailable()) { Why would you check it here again? We only setUpSigninButton(true) after checking GMS. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:603: switchToSignedMode(); Here is the only place to call "switchToSignedMode" now, we could remove this public function.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:229: * Refresh the list of available system accounts asynchronously. Maybe add that this is a convenience method that ignores the result? https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:241: * indicates whether something went wrong. Wait, so does true mean something did go wrong? Maybe "[...] indicates success value"? Also, maybe explain in more detail what it would mean if something went wrong? https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:276: if (mSignedIn) return; So we will never call the callback if we're signed in?
PTAL https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:229: * Refresh the list of available system accounts asynchronously. On 2017/01/10 00:00:16, Bernhard Bauer wrote: > Maybe add that this is a convenience method that ignores the result? Done. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:240: * @param callback Called once the accounts have been refreshed. Boolean On 2017/01/09 18:20:45, gogerald1 wrote: > Make it: > > /** > * Refresh the list of available system accounts asynchronously. > * > * @param callback Called once the accounts have been refreshed. Boolean > * indicates whether something went wrong. > */ Done. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:241: * indicates whether something went wrong. On 2017/01/10 00:00:16, Bernhard Bauer wrote: > Wait, so does true mean something did go wrong? Maybe "[...] indicates success > value"? Also, maybe explain in more detail what it would mean if something went > wrong? You're right, the comment is very confusing. I updated it, is it more clear? https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:251: callback.onResult(false); On 2017/01/09 18:20:45, gogerald1 wrote: > Thinking one situation: FRE forced sign in will return here if GMS is out of > date (only one account on device). When user come back to Chrome after updating > GMS (do not kill Chrome), the updateAccounts() is called without showing > confirmation screen directly. Hmm, that's a good point but I think it's better than the alternative (crashing). I thought about calling `mListener.onFailedToSetForcedAccount()` if we're in forced account mode here as well. However this doesn't work, because this will abort the FRE while it has a shown dialog (from checkGooglePlayServicesAvailable()). Android doesn't seem very happy about this. Do you have any idea? https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:276: if (mSignedIn) return; On 2017/01/10 00:00:16, Bernhard Bauer wrote: > So we will never call the callback if we're signed in? Yes. Note that line 244 there is the same check to avoid updateAccounts() to being called when the user is signed in. Without this check, I hit (flakily) a bad scenario which would wrongly update the sign-in button while on the sign-in confirmation page: * Call updateAccounts() twice in a row while in forced mode. * First updateAccounts() returns and auto sign-in the user. mSignedIn is now true. * Second updateAccounts() returns and acts as if the sign-in screen (and not the confirmation screen) is shown. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:497: if (!checkGooglePlayServicesAvailable()) { On 2017/01/09 18:20:45, gogerald1 wrote: > Why would you check it here again? We only setUpSigninButton(true) after > checking GMS. It isn't needed in this CL, but there is an issue here if GMS isn't up-to-date: AccountTrackerService.checkAndSeedSystemAccounts() will never call its listener so tapping sign-in will have no effect and block you on the page. Because in this CL we are still doing the GMS check on all Android versions in updateAccounts() it isn't needed. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:603: switchToSignedMode(); On 2017/01/09 18:20:45, gogerald1 wrote: > Here is the only place to call "switchToSignedMode" now, we could remove this > public function. True, but I'd like to keep this CL to a minimum, as it is fixing an M56 crash and I hope to get it merged (even though it's very late in the cycle).
https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:251: callback.onResult(false); On 2017/01/10 14:41:27, bzanotti wrote: > On 2017/01/09 18:20:45, gogerald1 wrote: > > Thinking one situation: FRE forced sign in will return here if GMS is out of > > date (only one account on device). When user come back to Chrome after > updating > > GMS (do not kill Chrome), the updateAccounts() is called without showing > > confirmation screen directly. > > Hmm, that's a good point but I think it's better than the alternative > (crashing). > > I thought about calling `mListener.onFailedToSetForcedAccount()` if we're in > forced account mode here as well. However this doesn't work, because this will > abort the FRE while it has a shown dialog (from > checkGooglePlayServicesAvailable()). Android doesn't seem very happy about this. > Do you have any idea? you probably can't call it on UI thread directly, try post a task on UI thread https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:497: if (!checkGooglePlayServicesAvailable()) { On 2017/01/10 14:41:27, bzanotti wrote: > On 2017/01/09 18:20:45, gogerald1 wrote: > > Why would you check it here again? We only setUpSigninButton(true) after > > checking GMS. > > It isn't needed in this CL, but there is an issue here if GMS isn't up-to-date: > AccountTrackerService.checkAndSeedSystemAccounts() will never call its listener > so tapping sign-in will have no effect and block you on the page. > > Because in this CL we are still doing the GMS check on all Android versions in > updateAccounts() it isn't needed. Not quite sure I understand the issue correctly, but you could keep it out of this CL if not needed especially this CL is expected to be merged to M56
LGTM when Gangui approves. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:276: if (mSignedIn) return; On 2017/01/10 14:41:27, bzanotti wrote: > On 2017/01/10 00:00:16, Bernhard Bauer wrote: > > So we will never call the callback if we're signed in? > > Yes. Note that line 244 there is the same check to avoid updateAccounts() to > being called when the user is signed in. > > Without this check, I hit (flakily) a bad scenario which would wrongly update > the sign-in button while on the sign-in confirmation page: > * Call updateAccounts() twice in a row while in forced mode. > * First updateAccounts() returns and auto sign-in the user. mSignedIn is now > true. > * Second updateAccounts() returns and acts as if the sign-in screen (and not the > confirmation screen) is shown. OK, thanks for the explanation. I think that warrants at least a comment: // If sign-in completed in the mean time, return in order to avoid showing the wrong state in the UI.
https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:251: callback.onResult(false); On 2017/01/10 20:29:24, gogerald1 wrote: > On 2017/01/10 14:41:27, bzanotti wrote: > > On 2017/01/09 18:20:45, gogerald1 wrote: > > > Thinking one situation: FRE forced sign in will return here if GMS is out of > > > date (only one account on device). When user come back to Chrome after > > updating > > > GMS (do not kill Chrome), the updateAccounts() is called without showing > > > confirmation screen directly. > > > > Hmm, that's a good point but I think it's better than the alternative > > (crashing). > > > > I thought about calling `mListener.onFailedToSetForcedAccount()` if we're in > > forced account mode here as well. However this doesn't work, because this will > > abort the FRE while it has a shown dialog (from > > checkGooglePlayServicesAvailable()). Android doesn't seem very happy about > this. > > Do you have any idea? > > you probably can't call it on UI thread directly, try post a task on UI thread It's not really a problem of UI thread, but of the "Update GMS" dialog not being closed properly and then leaking ("activity XXX has leaked window YYY"). I stumbled on another bug that I fixed, but the result is that Chrome keeps closing itself on startup, completely blocking the user out. This is because `mListener.onFailedToSetForcedAccount()` will abort the FRE which will close Chrome. This happens at each startup, and we don't have time to show the dialog to the user, so they don't know they need to upgrade GMS. ... I think staying the current state in the best short-term fix. We'll probably want to fix it more thoroughly at some point though. https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:276: if (mSignedIn) return; On 2017/01/11 10:47:08, Bernhard Bauer wrote: > On 2017/01/10 14:41:27, bzanotti wrote: > > On 2017/01/10 00:00:16, Bernhard Bauer wrote: > > > So we will never call the callback if we're signed in? > > > > Yes. Note that line 244 there is the same check to avoid updateAccounts() to > > being called when the user is signed in. > > > > Without this check, I hit (flakily) a bad scenario which would wrongly update > > the sign-in button while on the sign-in confirmation page: > > * Call updateAccounts() twice in a row while in forced mode. > > * First updateAccounts() returns and auto sign-in the user. mSignedIn is now > > true. > > * Second updateAccounts() returns and acts as if the sign-in screen (and not > the > > confirmation screen) is shown. > > OK, thanks for the explanation. I think that warrants at least a comment: > // If sign-in completed in the mean time, return in order to avoid showing the > wrong state in the UI. Done.
Thanks, lgtm https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2616643005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:251: callback.onResult(false); On 2017/01/11 16:15:33, bzanotti wrote: > On 2017/01/10 20:29:24, gogerald1 wrote: > > On 2017/01/10 14:41:27, bzanotti wrote: > > > On 2017/01/09 18:20:45, gogerald1 wrote: > > > > Thinking one situation: FRE forced sign in will return here if GMS is out > of > > > > date (only one account on device). When user come back to Chrome after > > > updating > > > > GMS (do not kill Chrome), the updateAccounts() is called without showing > > > > confirmation screen directly. > > > > > > Hmm, that's a good point but I think it's better than the alternative > > > (crashing). > > > > > > I thought about calling `mListener.onFailedToSetForcedAccount()` if we're in > > > forced account mode here as well. However this doesn't work, because this > will > > > abort the FRE while it has a shown dialog (from > > > checkGooglePlayServicesAvailable()). Android doesn't seem very happy about > > this. > > > Do you have any idea? > > > > you probably can't call it on UI thread directly, try post a task on UI thread > > It's not really a problem of UI thread, but of the "Update GMS" dialog not being > closed properly and then leaking ("activity XXX has leaked window YYY"). I > stumbled on another bug that I fixed, but the result is that Chrome keeps > closing itself on startup, completely blocking the user out. > > This is because `mListener.onFailedToSetForcedAccount()` will abort the FRE > which will close Chrome. This happens at each startup, and we don't have time to > show the dialog to the user, so they don't know they need to upgrade GMS. > > > ... > > I think staying the current state in the best short-term fix. We'll probably > want to fix it more thoroughly at some point though. Agreed, thanks for explaining in detail
The CQ bit was checked by bzanotti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2616643005/#ps40001 (title: "Comment early return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484152041267130,
"parent_rev": "447bdcac0898c480cf2de6e32a663c8b895dfb8a", "commit_rev":
"1bfabdd7fe941a66ae44746e0fed8fd34ad61302"}
Message was sent while issue was closed.
Description was changed from ========== [SignIn] Fix AccountSigninView for asynchronous updateAccounts() updateAccounts() now takes a callback to allow code to run once the accounts have actually been fetched. This also adds some check in updateAccounts() to ensure it only updates the accounts when expected (i.e. when the user isn't signed in). BUG=678543,672255 ========== to ========== [SignIn] Fix AccountSigninView for asynchronous updateAccounts() updateAccounts() now takes a callback to allow code to run once the accounts have actually been fetched. This also adds some check in updateAccounts() to ensure it only updates the accounts when expected (i.e. when the user isn't signed in). BUG=678543,672255 Review-Url: https://codereview.chromium.org/2616643005 Cr-Commit-Position: refs/heads/master@{#442932} Committed: https://chromium.googlesource.com/chromium/src/+/1bfabdd7fe941a66ae44746e0fed... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1bfabdd7fe941a66ae44746e0fed... |
