Fix bug in IdentityGetAuthToken / IdentityAPI relationship

https://codereview.chromium.org/2673443002 introduced a bug in the
Identity extension API implementation: it assumed that there would be
only one IdentityGetAuthToken instance that needed to listen for
IdentityAPI shutdown at a given time, but in fact, there can be many.
An example is if the user has multiple tabs open that each call the
getAuthToken() function with interactive set to true, so that they
end up hanging waiting for the user to authenticate.

The result of this bug is that if the browser shuts down with multiple
getAuthToken() calls active, only the last call will receive notice of
IdentityAPI shutdown. Other calls can then end up calling back into
Profile during its shutdown process, e.g. if they get notified by
OAuth2TokenService as part of *its* shutdown process that it has failed
their token requests. These calls can result in CHECKs (see crash
reports linked from crbug.com/720073).

This CL fixes the bug while maintaining the property that
IdentityGetAuthToken does not require an interface dependence on
IdentityAPI (i.e., the property that was the motivation of the original
CL). To do so, it changes IdentityAPI to maintain a CallbackList that
gets invoked on its shutdown.

This CL also introduces a browsertest that fails without this change.

Bug: 720073
Change-Id: I44daaa0c0bbc27e54da9ab826e256c70f3d5a2a8
Reviewed-on: https://chromium-review.googlesource.com/680974Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504289}

chrome/browser/extensions/api/identity/identity_api.cc

@@ -112,8 +112,7 @@ IdentityAPI::IdentityAPI(content::BrowserContext* context)
           LoginUIServiceFactory::GetShowLoginPopupCallbackForProfile(
               Profile::FromBrowserContext(context))),
       account_tracker_(&profile_identity_provider_,
-                       g_browser_process->system_request_context()),
-      get_auth_token_function_(nullptr) {
+                       g_browser_process->system_request_context()) {
   account_tracker_.AddObserver(this);
 }
 
@@ -155,8 +154,7 @@ const IdentityAPI::CachedTokens& IdentityAPI::GetAllCachedTokens() {
 }
 
 void IdentityAPI::Shutdown() {
-  if (get_auth_token_function_)
-    get_auth_token_function_->Shutdown();
+  on_shutdown_callback_list_.Notify();
   account_tracker_.RemoveObserver(this);
   account_tracker_.Shutdown();
 }

chrome/browser/extensions/api/identity/identity_api.h

@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback_list.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
@@ -39,8 +40,6 @@ class BrowserContext;
 
 namespace extensions {
 
-class IdentityGetAuthTokenFunction;
-
 class IdentityTokenCacheValue {
  public:
   IdentityTokenCacheValue();
@@ -101,9 +100,9 @@ class IdentityAPI : public BrowserContextKeyedAPI,
   void OnAccountSignInChanged(const gaia::AccountIds& ids,
                               bool is_signed_in) override;
 
-  void set_get_auth_token_function(
-      IdentityGetAuthTokenFunction* get_auth_token_function) {
-    get_auth_token_function_ = get_auth_token_function;
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  RegisterOnShutdownCallback(const base::Closure& cb) {
+    return on_shutdown_callback_list_.Add(cb);
   }
 
   // TODO(blundell): Eliminate this method once this class is no longer using
@@ -135,8 +134,7 @@ class IdentityAPI : public BrowserContextKeyedAPI,
 
   OnSignInChangedCallback on_signin_changed_callback_for_testing_;
 
-  // May be null.
-  IdentityGetAuthTokenFunction* get_auth_token_function_;
+  base::CallbackList<void()> on_shutdown_callback_list_;
 };
 
 template <>

chrome/browser/extensions/api/identity/identity_apitest.cc

@@ -86,13 +86,13 @@ namespace utils = extension_function_test_utils;
 static const char kAccessToken[] = "auth_token";
 static const char kExtensionId[] = "ext_id";
 
-class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
- protected:
-  // Asynchronous function runner allows tests to manipulate the browser window
-  // after the call happens.
-  void RunFunctionAsync(
-      UIThreadExtensionFunction* function,
-      const std::string& args) {
+// Asynchronous function runner allows tests to manipulate the browser window
+// after the call happens.
+class AsyncFunctionRunner {
+ public:
+  void RunFunctionAsync(UIThreadExtensionFunction* function,
+                        const std::string& args,
+                        content::BrowserContext* browser_context) {
     response_delegate_.reset(new api_test_utils::SendResponseHelper(function));
     std::unique_ptr<base::ListValue> parsed_args(utils::ParseList(args));
     EXPECT_TRUE(parsed_args.get()) <<
@@ -105,7 +105,7 @@ class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
       function->set_extension(empty_extension.get());
     }
 
-    function->set_browser_context(browser()->profile());
+    function->set_browser_context(browser_context);
     function->set_has_callback(true);
     function->RunWithValidation()->Execute();
   }
@@ -138,6 +138,28 @@ class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
   std::unique_ptr<api_test_utils::SendResponseHelper> response_delegate_;
 };
 
+class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
+ protected:
+  // Provide wrappers of AsynchronousFunctionRunner for convenience.
+  void RunFunctionAsync(UIThreadExtensionFunction* function,
+                        const std::string& args) {
+    async_function_runner_ = base::MakeUnique<AsyncFunctionRunner>();
+    async_function_runner_->RunFunctionAsync(function, args,
+                                             browser()->profile());
+  }
+
+  std::string WaitForError(UIThreadExtensionFunction* function) {
+    return async_function_runner_->WaitForError(function);
+  }
+
+  base::Value* WaitForSingleResult(UIThreadExtensionFunction* function) {
+    return async_function_runner_->WaitForSingleResult(function);
+  }
+
+ private:
+  std::unique_ptr<AsyncFunctionRunner> async_function_runner_;
+};
+
 class TestHangOAuth2MintTokenFlow : public OAuth2MintTokenFlow {
  public:
   TestHangOAuth2MintTokenFlow()
@@ -1300,7 +1322,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, InteractiveQueueShutdown) {
   EXPECT_FALSE(func->scope_ui_shown());
 
   // After the request is canceled, the function will complete.
-  func->Shutdown();
+  func->OnIdentityAPIShutdown();
   EXPECT_EQ(std::string(errors::kCanceled), WaitForError(func.get()));
   EXPECT_FALSE(func->login_ui_shown());
   EXPECT_FALSE(func->scope_ui_shown());
@@ -1318,7 +1340,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, NoninteractiveShutdown) {
   RunFunctionAsync(func.get(), "[{\"interactive\": false}]");
 
   // After the request is canceled, the function will complete.
-  func->Shutdown();
+  func->OnIdentityAPIShutdown();
   EXPECT_EQ(std::string(errors::kCanceled), WaitForError(func.get()));
 }
 
@@ -1480,6 +1502,62 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, ComponentWithNormalClientId) {
   EXPECT_EQ("client1", func->GetOAuth2ClientId());
 }
 
+// Ensure that IdentityAPI shutdown triggers an active function call to return
+// with an error.
+IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, IdentityAPIShutdown) {
+  scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
+  scoped_refptr<const Extension> extension(CreateExtension(CLIENT_ID | SCOPES));
+  func->set_extension(extension.get());
+  func->set_mint_token_result(TestOAuth2MintTokenFlow::MINT_TOKEN_SUCCESS);
+
+  // Have GetAuthTokenFunction actually make the request for the access token to
+  // ensure that the function doesn't immediately succeed.
+  func->set_auto_login_access_token(false);
+  RunFunctionAsync(func.get(), "[{}]");
+
+  id_api()->Shutdown();
+  EXPECT_EQ(std::string(errors::kCanceled), WaitForError(func.get()));
+}
+
+// Ensure that when there are multiple active function calls, IdentityAPI
+// shutdown triggers them all to return with errors.
+IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
+                       IdentityAPIShutdownWithMultipleActiveTokenRequests) {
+  // Set up two extension functions, having them actually make the request for
+  // the access token to ensure that they don't immediately succeed.
+  scoped_refptr<FakeGetAuthTokenFunction> func1(new FakeGetAuthTokenFunction());
+  scoped_refptr<const Extension> extension1(
+      CreateExtension(CLIENT_ID | SCOPES));
+  func1->set_extension(extension1.get());
+  func1->set_mint_token_result(TestOAuth2MintTokenFlow::MINT_TOKEN_SUCCESS);
+  func1->set_auto_login_access_token(false);
+
+  scoped_refptr<FakeGetAuthTokenFunction> func2(new FakeGetAuthTokenFunction());
+  scoped_refptr<const Extension> extension2(
+      CreateExtension(CLIENT_ID | SCOPES));
+  func2->set_extension(extension2.get());
+  func2->set_mint_token_result(TestOAuth2MintTokenFlow::MINT_TOKEN_SUCCESS);
+  func2->set_auto_login_access_token(false);
+
+  // Run both functions. Note that it's necessary to use AsyncFunctionRunner
+  // directly here rather than the AsyncExtensionBrowserTest instance methods
+  // that wrap it, as each AsyncFunctionRunner instance sets itself as the
+  // delegate of exactly one function.
+  AsyncFunctionRunner func1_runner;
+  func1_runner.RunFunctionAsync(func1.get(), "[{}]", browser()->profile());
+
+  AsyncFunctionRunner func2_runner;
+  func2_runner.RunFunctionAsync(func2.get(), "[{}]", browser()->profile());
+
+  // Shut down IdentityAPI and ensure that both functions complete with an
+  // error.
+  id_api()->Shutdown();
+  EXPECT_EQ(std::string(errors::kCanceled),
+            func1_runner.WaitForError(func1.get()));
+  EXPECT_EQ(std::string(errors::kCanceled),
+            func2_runner.WaitForError(func2.get()));
+}
+
 IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest, ManuallyIssueToken) {
   SignIn("primary@example.com");
 

chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc

@@ -218,15 +218,16 @@ void IdentityGetAuthTokenFunction::OnReceivedExtensionAccountInfo(
 void IdentityGetAuthTokenFunction::StartAsyncRun() {
   // Balanced in CompleteAsyncRun
   AddRef();
+
+  identity_api_shutdown_subscription_ =
       extensions::IdentityAPI::GetFactoryInstance()
           ->Get(GetProfile())
-      ->set_get_auth_token_function(this);
+          ->RegisterOnShutdownCallback(base::Bind(
+              &IdentityGetAuthTokenFunction::OnIdentityAPIShutdown, this));
 }
 
 void IdentityGetAuthTokenFunction::CompleteAsyncRun(bool success) {
-  extensions::IdentityAPI::GetFactoryInstance()
-      ->Get(GetProfile())
-      ->set_get_auth_token_function(nullptr);
+  identity_api_shutdown_subscription_.reset();
 
   SendResponse(success);
   Release();  // Balanced in StartAsyncRun
@@ -595,7 +596,7 @@ void IdentityGetAuthTokenFunction::OnGetTokenFailure(
 }
 #endif
 
-void IdentityGetAuthTokenFunction::Shutdown() {
+void IdentityGetAuthTokenFunction::OnIdentityAPIShutdown() {
   gaia_web_auth_flow_.reset();
   login_token_request_.reset();
   identity_manager_.reset();

chrome/browser/extensions/api/identity/identity_get_auth_token_function.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_IDENTITY_IDENTITY_GET_AUTH_TOKEN_FUNCTION_H_
 #define CHROME_BROWSER_EXTENSIONS_API_IDENTITY_IDENTITY_GET_AUTH_TOKEN_FUNCTION_H_
 
+#include "base/callback_list.h"
 #include "chrome/browser/extensions/api/identity/gaia_web_auth_flow.h"
 #include "chrome/browser/extensions/api/identity/identity_mint_queue.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
@@ -50,7 +51,7 @@ class IdentityGetAuthTokenFunction : public ChromeAsyncExtensionFunction,
     return token_key_.get();
   }
 
-  void Shutdown();
+  void OnIdentityAPIShutdown();
 
  protected:
   ~IdentityGetAuthTokenFunction() override;
@@ -193,6 +194,10 @@ class IdentityGetAuthTokenFunction : public ChromeAsyncExtensionFunction,
   IssueAdviceInfo issue_advice_;
   std::unique_ptr<GaiaWebAuthFlow> gaia_web_auth_flow_;
 
+  // Invoked when IdentityAPI is shut down.
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+      identity_api_shutdown_subscription_;
+
   identity::mojom::IdentityManagerPtr identity_manager_;
 };