Ensure we don't crash if user navigates back from NTP to Chrome sign-in page before it has fully lo…

chrome/browser/signin/signin_browsertest.cc

 // Copyright 2013 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.
 
 #ifndef CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_
 #define CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_
 
 #include "base/command_line.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/sync/sync_promo_ui.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_switches.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 
 namespace {
   const char kNonSigninURL[] = "www.google.com";
 }
 
 class SigninBrowserTest : public InProcessBrowserTest {
  public:
@@ skipping 121 matching lines @@
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   EXPECT_EQ(kOneClickSigninEnabled, signin->HasSigninProcess());
 
   // Navigating away should clear the sign-in process.
   GURL redirect_url("https://accounts.google.com/server-redirect?"
       "https://foo.com?service=chromiumsync");
   ui_test_utils::NavigateToURL(browser(), redirect_url);
   EXPECT_FALSE(signin->HasSigninProcess());
 }
 
+class BackOnNTPCommitObserver : public content::WebContentsObserver {
+ public:
+  explicit BackOnNTPCommitObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents) {
+  }
+
+  virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      content::PageTransition transition_type,
+      content::RenderViewHost* render_view_host) OVERRIDE {
+    if (url == GURL(chrome::kChromeUINewTabURL)) {
+      content::WindowedNotificationObserver observer(
+          content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+          content::NotificationService::AllSources());
+      web_contents()->GetController().GoBack();
+      observer.Wait();

[Roger Tawa OOO till Jul 10th, 2013/07/22 18:35:00]

For my own info, why do we need wait here?

[nasko, 2013/07/22 18:44:31]

This is to ensure that we don't continue execution until the navigation back to
the blank.html page has committed. If we don't wait here, DidStopLoading for NTP
will likely get delivered and if that happens, we can't simulate the race
condition.

+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BackOnNTPCommitObserver);
+};
+
+// This is a test for http://crbug.com/257277. It simulates the navigations
+// that occur if the user clicks on the "Skip for now" link at the signin page
+// and initiates a back navigation between the point of Commit and
+// DidStopLoading of the NTP.
+IN_PROC_BROWSER_TEST_F(SigninBrowserTest, SigninSkipForNowAndGoBack) {
+  GURL ntp_url(chrome::kChromeUINewTabURL);
+  GURL start_url =
+      SyncPromoUI::GetSyncPromoURL(SyncPromoUI::SOURCE_START_PAGE, true);
+  GURL skip_url(SyncPromoUI::GetSyncLandingURL("ntp", 1));
+
+  SigninManager* signin = SigninManagerFactory::GetForProfile(
+      browser()->profile());
+  EXPECT_FALSE(signin->HasSigninProcess());
+
+  ui_test_utils::NavigateToURL(browser(), start_url);
+  EXPECT_EQ(kOneClickSigninEnabled, signin->HasSigninProcess());
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Register an observer that will navigate back immediately on the commit of
+  // the NTP. This will allow us to hit the race condition of navigating back
+  // before the DidStopLoading message of NTP gets delivered.
+  BackOnNTPCommitObserver commit_observer(web_contents);
+
+  // Simulate clicking on the Skip for now link by navigating to the URL.
+  ui_test_utils::NavigateToURL(browser(), skip_url);

[Roger Tawa OOO till Jul 10th, 2013/07/22 18:35:00]

This call does a wait for the load to stop.  Why do we need another wait at line
229?

[nasko, 2013/07/22 18:44:31]

The extra wait is to ensure the test has enough time to execute the back
navigation, since it waits for the second redirect to NTP.

+
+  // Since the navigation to the blank URL is monitored for, the
+  // OneClickSigninHelper initiates immediately a navigation to the NTP.
+  // Thus, we expect the visible URL to be the NTP.
+  EXPECT_EQ(skip_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(ntp_url, web_contents->GetActiveURL());
+
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  observer.Wait();
+  EXPECT_EQ(skip_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(ntp_url, web_contents->GetActiveURL());

[Roger Tawa OOO till Jul 10th, 2013/07/22 18:35:00]

I'm not sure why these two tests are the same as lines 223-224.

[nasko, 2013/07/22 18:44:31]

Did you have a chance to read my timeline explanation I did earlier when Charlie
asked why this is the case?

+}
 #endif  // CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_