Reland cros: Test for overlapping ContinueSessionRestore calls

chrome/browser/chromeos/login/signin/oauth2_browsertest.cc

 // Copyright 2014 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 <string>
 #include <utility>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/signin/oauth2_login_manager.h"
 #include "chrome/browser/chromeos/login/signin/oauth2_login_manager_factory.h"
 #include "chrome/browser/chromeos/login/signin_specifics.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/login/test/oobe_base_test.h"
@@ skipping 12 matching lines @@
 #include "chromeos/login/auth/user_context.h"
 #include "components/app_modal/javascript_app_modal_dialog.h"
 #include "components/app_modal/native_app_modal_dialog.h"
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_store.h"
 #include "net/test/embedded_test_server/http_request.h"
@@ skipping 84 matching lines @@
 
   Profile* profile_;
   std::set<OAuth2LoginManager::SessionRestoreState> states_;
   bool waiting_for_state_;
   OAuth2LoginManager::SessionRestoreState final_state_;
   scoped_refptr<content::MessageLoopRunner> runner_;
 
   DISALLOW_COPY_AND_ASSIGN(OAuth2LoginManagerStateWaiter);
 };
 
+// Blocks a BrowserThread on construction and unblocks it on destruction.

[achuithb, 2017/02/02 00:37:10]

This seems like a generally useful class - I'm surprised there's not a utility
somewhere that does this.

[xiyuan, 2017/02/02 18:43:59]

I am surprised that I could not find similar class in test util code either.

+class BrowserThreadBlocker {
+ public:
+  explicit BrowserThreadBlocker(content::BrowserThread::ID identifier)
+      : unblock_event_(new base::WaitableEvent(
+            base::WaitableEvent::ResetPolicy::MANUAL,
+            base::WaitableEvent::InitialState::NOT_SIGNALED)) {
+    content::BrowserThread::PostTask(
+        identifier, FROM_HERE,
+        base::Bind(&BlockThreadOnThread, base::Owned(unblock_event_)));
+  }
+  ~BrowserThreadBlocker() { unblock_event_->Signal(); }

[achuithb, 2017/02/02 00:37:10]

Wonder if it's safer to use an explicit Release method instead of doing work in
the dtor?

[xiyuan, 2017/02/02 18:43:59]

Dtor should be safe as long as we don't have derived class etc. Doing it  in
dtor makes it work like a scoped object. IMHO, it is easier to use this way.

+
+ private:
+  // Blocks the target thread until |event| is signaled.
+  static void BlockThreadOnThread(base::WaitableEvent* event) { event->Wait(); }
+
+  // Owned by the BlockThreadOnThread callback.

[achuithb, 2017/02/02 00:37:10]

Maybe it's more accurate to say that this shared semaphore is deleted by the
callback?

[xiyuan, 2017/02/02 18:43:59]

Updated the comment to:
  // |unblock_event_| is deleted after BlockThreadOnThread returns.

+  base::WaitableEvent* const unblock_event_;
+  DISALLOW_COPY_AND_ASSIGN(BrowserThreadBlocker);

[achuithb, 2017/02/02 00:37:10]

I'd add a newline before this line

[xiyuan, 2017/02/02 18:43:59]

Done.

+};
+
 }  // namespace
 
 class OAuth2Test : public OobeBaseTest {
  protected:
   OAuth2Test() {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     OobeBaseTest::SetUpCommandLine(command_line);
 
     // Disable sync sinc we don't really need this for these tests and it also
@@ skipping 202 matching lines @@
     // Use capitalized and dotted user name on purpose to make sure
     // our email normalization kicks in.
     GetLoginDisplay()->ShowSigninScreenForCreds(kTestRawEmail,
                                                 kTestAccountPassword);
     session_start_waiter.Wait();
 
     if (wait_for_merge) {
       // Wait for the session merge to finish.
       WaitForMergeSessionCompletion(OAuth2LoginManager::SESSION_RESTORE_DONE);
     }
-}
+  }
 
+  OAuth2LoginManager::SessionRestoreStrategy GetSessionRestoreStrategy() {
+    OAuth2LoginManager* login_manager =
+        OAuth2LoginManagerFactory::GetInstance()->GetForProfile(profile());
+    return login_manager->restore_strategy_;
+  }
+
+ private:
   DISALLOW_COPY_AND_ASSIGN(OAuth2Test);
 };
 
 class CookieReader : public base::RefCountedThreadSafe<CookieReader> {
  public:
   CookieReader() {
   }
 
   void ReadCookies(Profile* profile) {
     context_ = profile->GetRequestContext();
@@ skipping 119 matching lines @@
       TryToLogin(AccountId::FromUserEmailGaiaId(kTestEmail, kTestGaiaId),
                  kTestAccountPassword));
 
   // Wait for the session merge to finish.
   WaitForMergeSessionCompletion(OAuth2LoginManager::SESSION_RESTORE_FAILED);
 
   EXPECT_EQ(GetOAuthStatusFromLocalState(account_id),
             user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);
 }
 
+// Sets up a new user with stored refresh token.
+IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_OverlappingContinueSessionRestore) {
+  StartNewUserSession(true);
+}
+
+// Tests that ContinueSessionRestore could be called multiple times.

[achuithb, 2017/02/02 00:37:10]

Impressive test.

+IN_PROC_BROWSER_TEST_F(OAuth2Test, OverlappingContinueSessionRestore) {
+  SetupGaiaServerForUnexpiredAccount();
+  SimulateNetworkOnline();
+
+  // Waits for login screen to be ready.
+  content::WindowedNotificationObserver(
+      chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
+      content::NotificationService::AllSources())
+      .Wait();
+
+  // Blocks DB thread to control TokenService::LoadCredentials timing.
+  std::unique_ptr<BrowserThreadBlocker> db_blocker =
+      base::MakeUnique<BrowserThreadBlocker>(content::BrowserThread::DB);
+
+  // Signs in as the existing user created in pre test.
+  EXPECT_TRUE(
+      TryToLogin(AccountId::FromUserEmailGaiaId(kTestEmail, kTestGaiaId),
+                 kTestAccountPassword));
+
+  // Session restore should be using the saved tokens.
+  EXPECT_EQ(OAuth2LoginManager::RESTORE_FROM_SAVED_OAUTH2_REFRESH_TOKEN,
+            GetSessionRestoreStrategy());
+
+  // Checks that refresh token is not yet loaded.
+  ProfileOAuth2TokenService* token_service =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile());
+  const std::string account_id =
+      PickAccountId(profile(), kTestGaiaId, kTestEmail);
+  EXPECT_FALSE(token_service->RefreshTokenIsAvailable(account_id));
+
+  // Invokes ContinueSessionRestore multiple times and there should be
+  // no DCHECK failures.
+  OAuth2LoginManager* login_manager =
+      OAuth2LoginManagerFactory::GetInstance()->GetForProfile(profile());
+  login_manager->ContinueSessionRestore();
+  login_manager->ContinueSessionRestore();
+
+  // Let go DB thread to finish TokenService::LoadCredentials.
+  db_blocker.reset();
+
+  // Session restore can finish normally and token is loaded.
+  WaitForMergeSessionCompletion(OAuth2LoginManager::SESSION_RESTORE_DONE);
+  EXPECT_TRUE(token_service->RefreshTokenIsAvailable(account_id));
+}
 
 const char kGooglePageContent[] =
     "<html><title>Hello!</title><script>alert('hello');</script>"
     "<body>Hello Google!</body></html>";
 const char kRandomPageContent[] =
     "<html><title>SomthingElse</title><body>I am SomethingElse</body></html>";
 const char kHelloPagePath[] = "/hello_google";
 const char kRandomPagePath[] = "/non_google_page";
 
 
@@ skipping 123 matching lines @@
     const GURL& server_url = embedded_test_server()->base_url();
     GURL::Replacements replace_google_host;
     replace_google_host.SetHostStr("www.google.com");
     GURL google_url = server_url.ReplaceComponents(replace_google_host);
     fake_google_page_url_ = google_url.Resolve(kHelloPagePath);
 
     GURL::Replacements replace_non_google_host;
     replace_non_google_host.SetHostStr("www.somethingelse.org");
     GURL non_google_url = server_url.ReplaceComponents(replace_non_google_host);
     non_google_page_url_ = non_google_url.Resolve(kRandomPagePath);
-}
+  }
 
-void SetUp() override {
-    embedded_test_server()->RegisterRequestHandler(
-        base::Bind(&FakeGoogle::HandleRequest,
-                   base::Unretained(&fake_google_)));
+  void SetUp() override {
+    embedded_test_server()->RegisterRequestHandler(base::Bind(
+        &FakeGoogle::HandleRequest, base::Unretained(&fake_google_)));
     OAuth2Test::SetUp();
   }
 
  protected:
   void UnblockMergeSession() {
     delayed_fake_gaia_->UnblockMergeSession();
   }
 
   void WaitForMergeSessionToStart() {
     delayed_fake_gaia_->WaitForMergeSessionToStart();
@@ skipping 140 matching lines @@
 
   if (!catcher.GetNextResult()) {
     std::string message = catcher.message();
     ADD_FAILURE() << "Tests failed: " << message;
   }
 
   EXPECT_TRUE(fake_google_.IsPageRequested());
 }
 
 }  // namespace chromeos