Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490093002:
Migrate opt-in auth flow to re-auth flow. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago Reviewers:
Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate opt-in auth flow to re-auth flow.
This CL migrates pre-fetch auth-token flow into re-auth flow.
So, internally, AuthHost implementation is split from
other arc container/enable statement management.
BUG=657687
BUG=b/31079732
TEST=Trybots. Ran on test device.
Committed: https://crrev.com/41e94820ce6ca738bdb2d319740ae79d875380e2
Cr-Commit-Position: refs/heads/master@{#432389}
Patch Set 1 #
Total comments: 16
Patch Set 2 : rebase #Patch Set 3 : Address comments #Patch Set 4 : Fix retry. #
Total comments: 5
Patch Set 5 : Rebase #Patch Set 6 : Workaround for restart case. #Patch Set 7 : Address comments. #Patch Set 8 : rebase #
Messages
Total messages: 22 (10 generated)
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL. Luis, as we chatted offline, let me pile the CLs. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (left): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:344: callback.Run(GetAndResetAuthCode()); Note: If you prefer, I can extract this part into another CL. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:914: DCHECK_EQ(state_, State::FETCHING_CODE); NOTE: This was actually wrong. This could be ACTIVE... https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:928: DCHECK_EQ(state_, State::FETCHING_CODE); Ditto.
first round https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (left): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:344: callback.Run(GetAndResetAuthCode()); On 2016/11/10 06:49:28, hidehiko wrote: > Note: If you prefer, I can extract this part into another CL. Actually can you log that this method is deprecated and callback.Run(std::string());? Let's stop maintaining this. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:802: VLOG(1) << "Starting ARC for first sign in."; Is there no way of knowing if this is the first sign-in? Are there other logs that already replace this? https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:124: const GetAuthCodeDeprecated0Callback& auth0_callback) This hasn't been used since M52. Let's get rid of it. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:392: account_info_notifier->Notify(false /* = is_enforced */, "", nit: s/""/std::string()/ https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:935: arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, |sign_in_time_| should always be updated in tandem with starting this timer. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1040: if (state_ != State::STOPPED) { Shouldn't this be always true? (|state_| should be State::ACTIVE here, and I don't see SetUIPage synchronously changing state). If it's not, please add a comment explaining since it's very surprising.
Thank you for review. PTAL. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (left): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:344: callback.Run(GetAndResetAuthCode()); On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > On 2016/11/10 06:49:28, hidehiko wrote: > > Note: If you prefer, I can extract this part into another CL. > > Actually can you log that this method is deprecated and > callback.Run(std::string());? Let's stop maintaining this. Acknowledged. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:802: VLOG(1) << "Starting ARC for first sign in."; On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > Is there no way of knowing if this is the first sign-in? Are there other logs > that already replace this? Moved to OnAndroidManagementChecked(). https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:124: const GetAuthCodeDeprecated0Callback& auth0_callback) On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > This hasn't been used since M52. Let's get rid of it. Extracted to crrev.com/2499933002. For this topic, let's discuss on the CL, although I imported the code in this CL to resolve the compile error. I'll just rebase if necessary. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:392: account_info_notifier->Notify(false /* = is_enforced */, "", On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > nit: s/""/std::string()/ Done. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:935: arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > |sign_in_time_| should always be updated in tandem with starting this timer. Good catch. Done. https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1040: if (state_ != State::STOPPED) { On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > Shouldn't this be always true? (|state_| should be State::ACTIVE here, and I > don't see SetUIPage synchronously changing state). > > If it's not, please add a comment explaining since it's very surprising. Unfortunately, there is a case this triggers. Commented.
https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1040: if (state_ != State::STOPPED) { On 2016/11/14 11:29:31, hidehiko wrote: > On 2016/11/10 23:58:20, Luis Héctor Chávez wrote: > > Shouldn't this be always true? (|state_| should be State::ACTIVE here, and I > > don't see SetUIPage synchronously changing state). > > > > If it's not, please add a comment explaining since it's very surprising. > > Unfortunately, there is a case this triggers. Commented. Oops, no. I was misunderstanding, and my test coverage was poor... Now the condition was fixed. https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1070: ShutdownBridge(); FYI: this implementation seems to hit some hidden race around session manager. ArcStopInstance DBus signal looks sent to Chrome twice asynchronously so that restarting is not yet working. I'll investigate it more tomorrow on my local TZ.
other than the two nits and the shutdown race investigation, this l-g-t-m. https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1069: SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16()); DCHECK_EQ(State::ACTIVE, state_); ? https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1082: // Otherwise, we restart the ARC. Note: this is the first boot case. nit: s/the ARC/ARC/g
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Turned out that the timing issue is actually caused by the combination of SessionManagerClient, ArcBridgeService(Impl), ArcSession(Impl), and ArcAuthService. Filed a bug crbug.com/665316 and added its workaround. PTAL. https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1069: SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16()); On 2016/11/14 17:21:36, Luis Héctor Chávez wrote: > DCHECK_EQ(State::ACTIVE, state_); ? Done. https://codereview.chromium.org/2490093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1082: // Otherwise, we restart the ARC. Note: this is the first boot case. On 2016/11/14 17:21:36, Luis Héctor Chávez wrote: > nit: s/the ARC/ARC/g Done.
lgtm
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2490093002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Migrate opt-in auth flow to re-auth flow. This CL migrates pre-fetch auth-token flow into re-auth flow. So, internally, AuthHost implementation is split from other arc container/enable statement management. BUG=657687 BUG=b/31079732 TEST=Trybots. Ran on test device. ========== to ========== Migrate opt-in auth flow to re-auth flow. This CL migrates pre-fetch auth-token flow into re-auth flow. So, internally, AuthHost implementation is split from other arc container/enable statement management. BUG=657687 BUG=b/31079732 TEST=Trybots. Ran on test device. Committed: https://crrev.com/41e94820ce6ca738bdb2d319740ae79d875380e2 Cr-Commit-Position: refs/heads/master@{#432389} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/41e94820ce6ca738bdb2d319740ae79d875380e2 Cr-Commit-Position: refs/heads/master@{#432389} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
