arc: Restore broken auth code requst on demand.

chrome/browser/chromeos/arc/arc_auth_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
@@ skipping 286 matching lines @@
 void ArcAuthService::GetAuthCodeDeprecated(
     const GetAuthCodeDeprecatedCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!IsOptInVerificationDisabled());
   callback.Run(GetAndResetAuthCode());
 }
 
 void ArcAuthService::GetAuthCode(const GetAuthCodeCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // GetAuthCodeAndAccountType operation must not be in progress.
-  DCHECK(!auth_account_callback_.is_null());
+  DCHECK(auth_account_callback_.is_null());
 
   const std::string auth_code = GetAndResetAuthCode();
   const bool verification_disabled = IsOptInVerificationDisabled();
   if (!auth_code.empty() || verification_disabled) {
     callback.Run(auth_code, !verification_disabled);
     return;
   }
 
   initial_opt_in_ = false;
   auth_callback_ = callback;
   StartUI();
 }
 
 void ArcAuthService::GetAuthCodeAndAccountType(
     const GetAuthCodeAndAccountTypeCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // GetAuthCode operation must not be in progress.
-  DCHECK(!auth_callback_.is_null());
+  DCHECK(auth_callback_.is_null());
 
   const std::string auth_code = GetAndResetAuthCode();
   const bool verification_disabled = IsOptInVerificationDisabled();
   if (!auth_code.empty() || verification_disabled) {
     callback.Run(auth_code, !verification_disabled,
                  mojom::ChromeAccountType::USER_ACCOUNT);
     return;
   }
 
   initial_opt_in_ = false;
   auth_account_callback_ = callback;
   StartUI();
 }
 
+bool ArcAuthService::IsAuthCodeRequest() {
+  return !auth_callback_.is_null() || !auth_account_callback_.is_null();
+}
+
 void ArcAuthService::OnSignInComplete() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::ACTIVE);
   DCHECK(!sign_in_time_.is_null());
 
   arc_sign_in_timer_.Stop();
 
   if (!IsOptInVerificationDisabled() &&
       !profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) {
     playstore_launcher_.reset(
@@ skipping 454 matching lines @@
   if (!IsArcManaged())
     profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
 }
 
 void ArcAuthService::DisableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
   profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
 }
 
 void ArcAuthService::StartUI() {

[hidehiko, 2016/10/13 15:22:00]

StartUI is called only from four places.
- GetAuthCode(AndAccountType).
- OnOptInPreferenceChanged.
- StartLso.

My understanding is;
- GetAuthCode(AndAccountType) is for re-sign in for some reason.
- OnOptInPreferenceChanged is the first ARC start up procedure.

However, StartLso sounds wierd because it is called only from ArcSupportHost's
onAgree event, which means the UI (= extension) should be already started at the
moment.
Thus, it should be somehow cleaned up and get rid of StartUI(). Then, the check
you added at L821 is unnecessary, because that if-statment can be moved to the
OnOptInPreferenceChanged.

[khmel, 2016/10/13 15:30:19]

Checks I added is real condition that prevents Android side for re-authenticate.
Re-auth request is called when Android is running and state is ACTIVE 

[hidehiko, 2016/10/13 15:44:32]

I meant;
- StartLso() should not use StartUI() conceptually because when StartLso() is
called the UI (= extension) is already started.
- Then, IsAuthCodeRequest() check is unnecessary to be here (or I'd say it looks
no-longer the part of StartUI() considering re-auth case), so you should move
the check to the caller (OnOptInPreferenceChanged()).

[khmel, 2016/10/13 15:53:21]

This is real case that blocks working re-auth process. I don't touch StartLso
code at all. StartUI is called from GetAuthCode. At this point UI is not shown
and I see this error.

[Yusuke Sato, 2016/10/15 01:16:32]

I'm wondering what's your request now based on the info Yury gave in the
previous comment. Split StartUI() into two (line 819-827 and 828-836), revert
line 821, and let GetAuthCode[AndAccountType] only call the latter, maybe?

[hidehiko, 2016/10/17 09:23:31]

Or, maybe introducing two functions.

void StartSupportExtension() {  // or any better name?
  if (arc is stopped()) {
    // show error.
    return;
  }
  SetState(State::FETCHING_CODE);
  ShowUI(UIPage::TERMS_PROGRESS, ...);
}

void PrepareAuthContext() {  // Any better name... ArcAuthContext looks not a
context, actually???
  SetState(State::FETCHING_CODE);  // Is this and starting extension in really
the same state...?
  context_->PrepareContext();
}

And call necessary ones for each.
// Note: More TODOs are there on those functions still, but I dropped them from
this comment, because it looks out of this CL's focus, I think.

   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (!arc_bridge_service()->stopped()) {
+  if (!IsAuthCodeRequest() && !arc_bridge_service()->stopped()) {
     // If the user attempts to re-enable ARC while the bridge is still running
     // the user should not be able to continue until the bridge has stopped.
-    ShowUI(UIPage::ERROR, l10n_util::GetStringUTF16(
-                              IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR));

[khmel, 2016/10/12 23:46:28]

This error has nothing with "Couldn't reach Google Play. Try again now. " and
may be very confusing while analyzing user reports. Moreover it is hard to image
how it is possible reach this code. If user re-enable Arc, this service passes
through Stopped state and this means stopping the bridge. I would probably
change this to NOTREACHED().

+    ShowUI(UIPage::ERROR,
+           l10n_util::GetStringUTF16(IDS_ARC_SIGN_IN_UNKNOWN_ERROR));
     return;
   }
 
   SetState(State::FETCHING_CODE);
 
   if (initial_opt_in_) {
     initial_opt_in_ = false;
     ShowUI(UIPage::TERMS_PROGRESS, base::string16());
   } else {
     context_->PrepareContext();
@@ skipping 65 matching lines @@
           l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
       UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
       break;
     default:
       NOTREACHED();
   }
 }
 
 void ArcAuthService::StartArcIfSignedIn() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (state_ == State::ACTIVE)
+  if (state_ == State::ACTIVE && !IsAuthCodeRequest())

[hidehiko, 2016/10/13 15:22:00]

My understanding is that; ArcAuthService shows an extension UI if;
- this is the first time the user starts ARC on the device, or
- For some reason (what's the case, BTW?), re-sign in is requested from ARC.

Is there any other cases?
If these are *only* two, the condition looks slightly weird to me because;
- For the first case, ARC should still be in stopped state.
- For the second case, ARC should be running, but may be stopped due to, e.g.,
crash.

Then, what's the case if ARC is running, but not in the re-sign in flow?

[khmel, 2016/10/13 15:30:19]

It can be requested when Arc is running on second or next boot and Android needs
re-auth or authenticate. In the last case, state_ is active.

[hidehiko, 2016/10/13 15:44:31]

I understand |state_| can be ACTIVE in re-auth case.
My question is, what is the case where |state_| is ACTIVE but *not* in re-auth
case?

[khmel, 2016/10/13 15:53:21]

I don't see such case. I only see case with re-auth request. Note,
StartArcIfSigned probably not the best function name in this case.

[Yusuke Sato, 2016/10/15 01:16:32]

Then we can revert line 911 to the original one?

[hidehiko, 2016/10/17 09:23:31]

Reverting will cause a problem, because the new code actually would like not to
return if IsAuthCodeRequest() == true even if state_ == ACTIVE.
Maybe remove entirely instead, and D?CHECK etc. with descriptive comments, if
there is no case that state_ == ACTIVE && !IsAuthCodeRequest()?

     return;
 
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) ||
-      IsOptInVerificationDisabled()) {
+  if (!IsAuthCodeRequest() &&

[hidehiko, 2016/10/13 15:22:00]

In this case, kArcSignedIn should be set to false already, shouldn't it?
IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC. It sounds
the real sign-in condition and persistent kArcSignedIn preference is
conflicting?

[khmel, 2016/10/13 15:30:19]

If AuthCodeRequest is set then kArcSignedIn may be on or off, there is no
requirements here for it. GetAuthCode can be called at any time, during first
boot or on consequence boots. There is also one more flag added here to support
the case OOBE integration.

[hidehiko, 2016/10/13 15:44:32]

Isn't kArcSignedIn represents the ARC container sign-in status?
If yes, and re-auth request means that the container is failed to sign-in, the
preference should be set to false to keep the state in sync, at the first of
re-auth request.

[khmel, 2016/10/13 15:53:21]

Re-auth may occur at any moment, with or without SignedIn status. Please note
that we might have flag skip auth. In this case SignedIn status would be set
without actual signing on Android side. You may consider SignedIn state as one
successful run of Android

[Yusuke Sato, 2016/10/15 01:16:32]

Ok, so we don't have to fix kArcSignedIn behavior at least in this CL, correct?
Assuming so, I guess hidehiko's overall concern on this CL is that changing
functions' behavior depending on the auth_*callback_ state (null or not) is
brittle and may incur a maintenance burden. I kind of agree with that.

Given that we can remove all other IsAuthCodeRequest() now (I hope), is there a
not-too-complicated-and-M55-merge-able way to get rid of the remaining
IsAuthCodeRequest() call at line 914?

My (somewhat uninformed) guess is that the function is called only with either
the following paths:

 GetAuthCode*()
   ...snip...
     OnContextReady()
       CheckAndroidManagement() with |background_mode| == false
         StartArcIfSignedIn()

 -or-

 GetAuthCode*()
   ...snip...
     OnContextReady()
       CheckAndroidManagement() with |background_mode| == false
         ArcAndroidManagementChecker()'s ctor
           ...snip...
             StartArcIfSignedIn()

correct? And |background_mode| indicates if it's re-auth or not. If so, can we
propagate |background_mode| to StartArcIfSignedIn() so that the function can
know whether or not it's called during re-auth? WDYT? Does this address your
concern?

[hidehiko, 2016/10/17 09:23:31]

https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?q=kArcSigned...

explicitly says it is corresponding to the Android's sign in state. So, if the
semantics of the value is changed, it should be updated. Though, I would suggest
not to change the semantics, because the value is persistent and old values can
be inherited, so the Chrome update would cause a trouble I think.

If the semantics is kept as is, we need to set the value to false at the
beginning of GetAuthCode*()?

BTW, I couldn't track it. Did the semantics change happen already? Or it is
under plan?

+      (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) ||
+       IsOptInVerificationDisabled())) {
     StartArc();
   } else {
     const base::CommandLine* command_line =
         base::CommandLine::ForCurrentProcess();
     std::string auth_endpoint;
     if (command_line->HasSwitch(chromeos::switches::kArcUseAuthEndpoint)) {
       auth_endpoint = command_line->GetSwitchValueASCII(
           chromeos::switches::kArcUseAuthEndpoint);
     }
 
@@ skipping 16 matching lines @@
       return os << kStateFetchingCode;
     case ArcAuthService::State::ACTIVE:
       return os << kStateActive;
     default:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc