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"
 
+

[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";
 }
 
 class SigninBrowserTest : public InProcessBrowserTest {
  public:
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     https_server_.reset(new net::SpawnedTestServer(
         net::SpawnedTestServer::TYPE_HTTPS,
         net::SpawnedTestServer::kLocalhost,
@@ skipping 117 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 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_