arc: Fix situation when ARC cannot be disabled safely.

chrome/browser/chromeos/arc/arc_session_manager.cc

Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc b/chrome/browser/chromeos/arc/arc_session_manager.cc
index 3544f007a7645d368c16cdb8ec58aa1cd3f15a64..b23ddc4b043006774b9fa4de95be413ba71caa56 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -282,6 +282,14 @@ void ArcSessionManager::MaybeReenableArc() {
 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.
@@ -293,7 +301,7 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
     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;
   }