arc: Fix situation when ARC cannot be disabled safely.

chrome/browser/chromeos/arc/arc_session_manager.cc

 // Copyright 2016 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_session_manager.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
@@ skipping 264 matching lines @@
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
   // the ARC user data wipe is still required or not.
   reenable_arc_ = false;
   VLOG(1) << "Reenable ARC";
   EnableArc();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // Due the async nature of closing mojo channel, some sign in error may arrive

[hidehiko, 2017/01/06 14:25:36]

"some sign in error" sounds ambiguous, because in future an engineer cannot
figure out what error is expected and what is not.

Maybe: If the Mojo message to notify finishing the provisioning is already sent
from the container, it will be processed even after requesting to stop the
container.

Or something?

[khmel, 2017/01/10 02:17:12]

Done.

+  // after stopping ARC. Ignore all results arriving after disabling ARC.

[hidehiko, 2017/01/06 14:25:36]

In precise, "after disabling ARC" sounds misleading, because this does not
support "disabling then reenabling" case. So, maybe:

"Ignore all |result|s arriving while ARC is disabled, in order to avoid popping
up an error message triggered below."

should be less confusing.

[khmel, 2017/01/10 02:17:12]

Done.

+  if (!IsArcEnabled()) {

[hidehiko, 2017/01/06 14:25:36]

Please also explicitly comment that this code intentionally does not support the
case of reenabling.
Otherwise, if an engineer in future read this code, they would misunderstand
this as an unintentional bug.

[khmel, 2017/01/10 02:17:12]

Done.

+    LOG(WARNING) << "Provisioning result received after Arc was disabled. "
+                 << " Ignoring result " << static_cast<int>(result) << ".";

[hidehiko, 2017/01/06 14:25:36]

Please address previous comment:
, too.

[khmel, 2017/01/10 02:17:12]

Done.

+    return;
+  }
+
   // Due asynchronous nature of stopping the ARC instance,
   // OnProvisioningFinished may arrive after setting the |State::STOPPED| state
   // and |State::Active| is not guaranteed to be set here.
   // prefs::kArcDataRemoveRequested also can be active for now.
 
   if (provisioning_reported_) {
     // We don't expect ProvisioningResult::SUCCESS is reported twice or reported
     // after an error.
     DCHECK_NE(result, ProvisioningResult::SUCCESS);
     // TODO (khmel): Consider changing LOG to NOTREACHED once we guaranty that
     // no double message can happen in production.
-    LOG(WARNING) << " Provisioning result was already reported. Ignoring "
+    LOG(WARNING) << "Provisioning result was already reported. Ignoring "
                  << " additional result " << static_cast<int>(result) << ".";
     return;
   }
   provisioning_reported_ = true;
 
   if (result == ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR) {
     if (IsArcKioskMode()) {
       VLOG(1) << "Robot account auth code fetching error";
       // Log out the user. All the cleanup will be done in Shutdown() method.
       // The callback is not called because auth code is empty.
@@ skipping 679 matching lines @@
       return os << "ACTIVE";
   }
 
   // Some compiler reports an error even if all values of an enum-class are
   // covered indivisually in a switch statement.
   NOTREACHED();
   return os;
 }
 
 }  // namespace arc