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 269 matching lines @@
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // 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 (!IsArcEnabled()) {

[hidehiko, 2017/01/05 08:18:02]

This check does not work in more complicated situation, unfortunately.
E.g.; if "ARC is disabled, then reenabled, and then OnProvisioningFinished is
called", this check passes through.

Could you comment that this check is short-term workaround meanwhile,
and what situation this is trying to address?

[khmel, 2017/01/05 22:55:53]

Done.

+    LOG(WARNING) << " Provisioning result received after Arc was disabled. "

[hidehiko, 2017/01/05 08:18:02]

Please remove a space between '"' and P. Ditto for '"' and I at the line below.

Optional: How about raise the severity to ERROR, considering;
1) This is actually the case we should not hit (although, due to the current
code structure, it is difficult to fix quickly).
2) This should be very rare, IMHO. ("Disabling ARC checkbox manually sounds not
a common user action" multiplies "It needs to be done while provisioning and to
happen the racy situation").

[khmel, 2017/01/05 22:55:53]

The reason of this situation is the same as for line 302. So I would prefer
WARNING here to be consistent. Imho this is not rare case. This happens always
when ARC is disabled while requesting auth code and this is how mojo channel
work. 

+                 << " Ignoring result " << static_cast<int>(result) << ".";
+    return;
+  }
+
   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 "
                  << " additional result " << static_cast<int>(result) << ".";
     return;
   }
@@ skipping 686 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