Fix race issue in ArcAuthService.

chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc

Index: chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc
diff --git a/chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc b/chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2d97ead2bee97b68f4a6a4fee1e4eaae50ccf99c
--- /dev/null
+++ b/chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc
@@ -0,0 +1,69 @@
+// 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/auth/arc_manual_auth_code_fetcher.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "chrome/browser/chromeos/arc/arc_auth_context.h"
+
+namespace arc {
+
+ArcManualAuthCodeFetcher::ArcManualAuthCodeFetcher(ArcAuthContext* context,
+                                                   ArcSupportHost* support_host)
+    : context_(context), support_host_(support_host), weak_ptr_factory_(this) {
+  DCHECK(context_);
+  DCHECK(support_host_);
+  support_host_->AddObserver(this);
+}
+
+ArcManualAuthCodeFetcher::~ArcManualAuthCodeFetcher() {
+  support_host_->RemoveObserver(this);
+}
+
+void ArcManualAuthCodeFetcher::Fetch(const FetchCallback& callback) {
+  DCHECK(pending_callback_.is_null());
+  pending_callback_ = callback;
+
+  FetchInternal();
+}
+
+void ArcManualAuthCodeFetcher::FetchInternal() {
+  DCHECK(!pending_callback_.is_null());
+  DCHECK(!preparing_context_);
+  preparing_context_ = true;
+  context_->Prepare(base::Bind(&ArcManualAuthCodeFetcher::OnContextPrepared,
+                               weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ArcManualAuthCodeFetcher::OnContextPrepared(
+    net::URLRequestContextGetter* request_context_getter) {
+  DCHECK(!pending_callback_.is_null());
+  preparing_context_ = false;
+  if (!request_context_getter) {
+    support_host_->ShowError(ArcSupportHost::Error::SIGN_IN_NETWORK_ERROR,
+                             false);
+    return;
+  }
+
+  support_host_->ShowLso();
+}
+
+void ArcManualAuthCodeFetcher::OnAuthSucceeded(const std::string& auth_code) {
+  DCHECK(!pending_callback_.is_null());
+  base::ResetAndReturn(&pending_callback_).Run(auth_code);
+}
+
+void ArcManualAuthCodeFetcher::OnRetryClicked() {
+  DCHECK(!pending_callback_.is_null());
+  // If a user clicks "TRY AGAIN" button very quickly, this can be called
+  // twice or more consecutively. Then, do not run the FetchInternal(), because
+  // there should be an inflight operation.
+  if (preparing_context_)
+    return;
+  FetchInternal();

[Luis Héctor Chávez, 2016/12/03 01:27:54]

Don't we want to transition to another page at this point such that even if the
user clicks "TRY AGAIN" not too quickly, it is possible to have several such
events in the queue and some of them being interleaved with the callback from
OnContextPrepared? (thus working around the check in L64).

[Luis Héctor Chávez, 2016/12/03 14:24:26]

i thought about this a bit more: that would still be susceptible to races. how
about you disable the button once it's pressed from js so that it's impossible
for this scenario to happen? you can still transition to the "just one sec..."
page here just to give feedback in case the context creation takes a bit longer
than expected.

[hidehiko, 2016/12/05 06:00:31]

SGTM.

Can I work on the UI change in another CL, because 1) the change would have
slightly different scope from this CL's main goal, 2) it is not regression by
this change IIUC, and 3) it conflicts with the M56 bug fix, unfortunately.

I removed the |preparing_context_| and related checks.

+}
+
+}  // namespace arc