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

Index: chrome/browser/signin/signin_browsertest.cc
diff --git a/chrome/browser/signin/signin_browsertest.cc b/chrome/browser/signin/signin_browsertest.cc
index 14ee972e3f4d8491e9a2b46cb2d357a14cfcf87f..332ba4e5dbc7084434aad2ccc5c629d1e1aa47dd 100644
--- a/chrome/browser/signin/signin_browsertest.cc
+++ b/chrome/browser/signin/signin_browsertest.cc
@@ -17,9 +17,12 @@
 #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"
@@ -161,4 +164,70 @@ IN_PROC_BROWSER_TEST_F(SigninBrowserTest, NotTrustedAfterRedirect) {
   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();
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BackOnNTPCommitObserver);
+};
+
+// This is a test for http://crbug.com/257277. It simulates the navigations
+// that occur of the user clicks on the "Skip for now" link at the signin page

[Charlie Reis, 2013/07/19 23:28:43]

nit: of -> if

[nasko, 2013/07/22 16:18:28]

Done.

+// 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);
+
+  // 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();

[Charlie Reis, 2013/07/19 23:28:43]

This looks potentially flaky to me.  We could get LOAD_STOP before getting here
and then we'd wait forever.

[nasko, 2013/07/22 16:18:28]

I don't think we can, see the next comment for more details.

+  EXPECT_EQ(skip_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(ntp_url, web_contents->GetActiveURL());

[Charlie Reis, 2013/07/19 23:28:43]

Sorry, this still doesn't make sense to me.

In the new code, when we commit the NTP, we synchronously delete the blank
entry, right?  That means going back would put us back on the start_url, not
skip_url.

[nasko, 2013/07/22 16:18:28]

Here is the timeline of events:

Click on "Skip for now", which goes to blank.html
Commit blank.html
DidStopLoading blank.html - this creates the history cleaner and does OpenURL
for NTP. At this point we have last committed of blank.html and visible of NTP.
Commit NTP - at this point the observer I've added initiates a back navigation
to blank.html. The pending navigation entry and visible URL is for blank.html,
while last committed is NTP. Due to the changes in nav controller, we will not
delete the history entry here, since it is the pending one.
DidStopLoading NTP - nothing really done here
Commit blank.html
DidStopLoading blank.html - this creates another history cleaner and is what the
LOAD_STOP observer sees. At this point we are back to the last committed is
blank.html and since the signin helper does OpenURL on NTP, we get it as the
visible URL.

I hope this clears things up. I can remove the extra URL checks after the
observer, but to me it is a way to ensure we understand what is going on and if
something changes, we can adjust the test to match it.

[Charlie Reis, 2013/07/22 23:18:07]

How is that possible?  It doesn't seem like the test should be able to do a back
navigation to blank.html before the CurrentHistoryCleaner deletes it.  Are you
saying the BackOnNTPCommitObserver fires before the CurrentHistoryCleaner?
I was hoping to prevent the creation of another history cleaner by forcing the
blank.html entry to be immediately deleted when the NTP commits.  Is that not
happening?

[nasko, 2013/07/23 20:34:47]

I've modified the patch to use DidCommitProvisionalLoadForFrame in order to
ensure we don't create multiple history cleaners. If we do it in
DidNavigateMainFrame, there is a chance for someone observing the commit to
navigate back, just like my test was doing.

+}
 #endif  // CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_