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..ade222cf51630cbbbd20354eb9bf05ace186d4fd 100644
--- a/chrome/browser/signin/signin_browsertest.cc
+++ b/chrome/browser/signin/signin_browsertest.cc
@@ -17,13 +17,17 @@
 #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"
 
+

[Charlie Reis, 2013/07/18 21:53:38]

nit: Remove extra line.

[nasko, 2013/07/19 23:03:41]

Done.

 namespace {
   const char kNonSigninURL[] = "www.google.com";
 }
@@ -161,4 +165,71 @@ IN_PROC_BROWSER_TEST_F(SigninBrowserTest, NotTrustedAfterRedirect) {
   EXPECT_FALSE(signin->HasSigninProcess());
 }
 
+class CommitWebContentsObserver : public content::WebContentsObserver {

[Charlie Reis, 2013/07/18 21:53:38]

Maybe BackOnNTPCommitObserver?  CommitWebContentsObserver doesn't really say
what it does.

[nasko, 2013/07/19 23:03:41]

Done.

+ public:
+  CommitWebContentsObserver(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)) {
+      {

[Charlie Reis, 2013/07/18 21:53:38]

nit: Extra braces not needed.

[nasko, 2013/07/19 23:03:41]

Yeah, a few things are left over from the mess that this code used to be : )

+        content::WindowedNotificationObserver observer(
+            content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+            content::NotificationService::AllSources());
+        web_contents()->GetController().GoBack();
+        observer.Wait();
+      }
+    }
+  }
+
+ private:
+  Browser* browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(CommitWebContentsObserver);
+};
+
+IN_PROC_BROWSER_TEST_F(SigninBrowserTest, SkipForNowAndGoBack) {

[Charlie Reis, 2013/07/18 21:53:38]

nit: Add a comment referencing the bug.

[nasko, 2013/07/19 23:03:41]

Done.

+  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();
+
+  // 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 causes a "redirect" to NTP, we expect

[Charlie Reis, 2013/07/18 21:53:38]

How do they cause this "redirect"?  Is it started immediately when the skip_url
commits?

[nasko, 2013/07/19 23:03:41]

Done.

+  // the visible URL to be the NTP.
+  EXPECT_EQ(skip_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(ntp_url, web_contents->GetActiveURL());

[Charlie Reis, 2013/07/18 21:53:38]

It's GetVisibleURL now, right?  :)

[nasko, 2013/07/19 23:03:41]

Sadly, not yet : (. But I put the comment intentionally like this, so I don't
miss it when this changes : ).

+
+  // 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.

[Charlie Reis, 2013/07/18 21:53:38]

Nice!  That sounds like the right approach to me, since DidStopLoading happens
asynchronously.

[nasko, 2013/07/19 23:03:41]

Done.

+  CommitWebContentsObserver commit_observer(web_contents);

[Charlie Reis, 2013/07/18 21:53:38]

Are we guaranteed that the NTP navigation can't commit before here, or do we
need to put this declaration above the NavigateToURL call on line 214?

[nasko, 2013/07/19 23:03:41]

I can't guarantee anything in complex systems ;) so I'm moving the observer
declaration before the navigation, to be on the safer side.

+
+  {

[Charlie Reis, 2013/07/18 21:53:38]

nit: Braces aren't needed here.  They're mainly used in tests where we repeat
this block several times with the same variable names.

[nasko, 2013/07/19 23:03:41]

Done.

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

[Charlie Reis, 2013/07/18 21:53:38]

I don't understand this part.  It looks like the state we were in on line 218. 
Has the NTP committed?  Have we gone back?  Does that trigger another NTP load? 
Could other DidStopLoading calls have happened along the way?

[nasko, 2013/07/19 23:03:41]

Since we navigated back during the NTP commit, we are back in the same state
once we have the navigation complete. On DidStopLoading of the blank page, the
helper object will initiate another load of the NTP, so we are back in the same
point.

+  }
+}
 #endif  // CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_