Avoid calling PerformNavigation when the delegate is null.

This CL avoid calling PerformNavigation on the delegate when it is null. It also makes the SigninViewController's delegate private to avoid it being accessed by other classes and exposing it to similar crashes. Bug: 758830 Change-Id: Ic778de6957115855273e6137721b0c0ace1315a6 Reviewed-on: https://chromium-review.googlesource.com/647889 Commit-Queue: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#499929}
nwjs · Sep 6, 2017 · c4d3dc1 · c4d3dc1
1 parent f7f2f63
commit c4d3dc1
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 57 deletions.
diff --git a/chrome/browser/ui/signin_view_controller.cc b/chrome/browser/ui/signin_view_controller.cc
@@ -7,8 +7,7 @@
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/signin_view_controller_delegate.h"
 
-SigninViewController::SigninViewController()
-    : signin_view_controller_delegate_(nullptr) {}
+SigninViewController::SigninViewController() : delegate_(nullptr) {}
 
 SigninViewController::~SigninViewController() {
   CloseModalSignin();
@@ -21,24 +20,22 @@ void SigninViewController::ShowModalSignin(
   CloseModalSignin();
   // The delegate will delete itself on request of the UI code when the widget
   // is closed.
-  signin_view_controller_delegate_ =
-      SigninViewControllerDelegate::CreateModalSigninDelegate(
-          this, mode, browser, access_point);
+  delegate_ = SigninViewControllerDelegate::CreateModalSigninDelegate(
+      this, mode, browser, access_point);
 
   // When the user has a proxy that requires HTTP auth, loading the sign-in
   // dialog can trigger the HTTP auth dialog.  This means the signin view
   // controller needs a dialog manager to handle any such dialog.
-  signin_view_controller_delegate_->AttachDialogManager();
+  delegate_->AttachDialogManager();
   chrome::RecordDialogCreation(chrome::DialogIdentifier::SIGN_IN);
 }
 
 void SigninViewController::ShowModalSyncConfirmationDialog(Browser* browser) {
   CloseModalSignin();
   // The delegate will delete itself on request of the UI code when the widget
   // is closed.
-  signin_view_controller_delegate_ =
-      SigninViewControllerDelegate::CreateSyncConfirmationDelegate(this,
-                                                                   browser);
+  delegate_ = SigninViewControllerDelegate::CreateSyncConfirmationDelegate(
+      this, browser);
   chrome::RecordDialogCreation(
       chrome::DialogIdentifier::SIGN_IN_SYNC_CONFIRMATION);
 }
@@ -47,25 +44,34 @@ void SigninViewController::ShowModalSigninErrorDialog(Browser* browser) {
   CloseModalSignin();
   // The delegate will delete itself on request of the UI code when the widget
   // is closed.
-  signin_view_controller_delegate_ =
+  delegate_ =
       SigninViewControllerDelegate::CreateSigninErrorDelegate(this, browser);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::SIGN_IN_ERROR);
 }
 
+bool SigninViewController::ShowsModalDialog() {
+  return delegate_ != nullptr;
+}
+
 void SigninViewController::CloseModalSignin() {
-  if (signin_view_controller_delegate_)
-    signin_view_controller_delegate_->CloseModalSignin();
+  if (delegate_)
+    delegate_->CloseModalSignin();
 
-  DCHECK(!signin_view_controller_delegate_);
+  DCHECK(!delegate_);
 }
 
 void SigninViewController::SetModalSigninHeight(int height) {
-  if (signin_view_controller_delegate_)
-    signin_view_controller_delegate_->ResizeNativeView(height);
+  if (delegate_)
+    delegate_->ResizeNativeView(height);
+}
+
+void SigninViewController::PerformNavigation() {
+  if (delegate_)
+    delegate_->PerformNavigation();
 }
 
 void SigninViewController::ResetModalSigninDelegate() {
-  signin_view_controller_delegate_ = nullptr;
+  delegate_ = nullptr;
 }
 
 // static
@@ -75,3 +81,9 @@ bool SigninViewController::ShouldShowModalSigninForMode(
          mode == profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT ||
          mode == profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH;
 }
+
+content::WebContents*
+SigninViewController::GetModalDialogWebContentsForTesting() {
+  DCHECK(delegate_);
+  return delegate_->web_contents();
+}
diff --git a/chrome/browser/ui/signin_view_controller.h b/chrome/browser/ui/signin_view_controller.h
@@ -11,6 +11,14 @@
 class Browser;
 class SigninViewControllerDelegate;
 
+namespace content {
+class WebContents;
+}
+
+namespace login_ui_test_utils {
+class SigninViewControllerTestUtil;
+}
+
 namespace signin_metrics {
 enum class AccessPoint;
 }
@@ -35,23 +43,31 @@ class SigninViewController {
   void ShowModalSyncConfirmationDialog(Browser* browser);
   void ShowModalSigninErrorDialog(Browser* browser);
 
+  // Returns true if the modal dialog is shown.
+  bool ShowsModalDialog();
+
   // Closes the tab-modal signin flow previously shown using this
   // SigninViewController, if one exists. Does nothing otherwise.
   void CloseModalSignin();
 
   // Sets the height of the modal signin dialog.
   void SetModalSigninHeight(int height);
 
-  // Notifies this object that it's |signin_view_controller_delegate_|
-  // member has become invalid.
-  void ResetModalSigninDelegate();
+  // Either navigates back in the signin flow if the history state allows it or
+  // closes the flow otherwise.
+  // Does nothing if the signin flow does not exist.
+  void PerformNavigation();
 
-  SigninViewControllerDelegate* delegate() {
-    return signin_view_controller_delegate_;
-  }
+  // Notifies this object that it's |delegate_| member has become invalid.
+  void ResetModalSigninDelegate();
 
  private:
-  SigninViewControllerDelegate* signin_view_controller_delegate_;
+  friend class login_ui_test_utils::SigninViewControllerTestUtil;
+
+  // Returns the web contents of the modal dialog.
+  content::WebContents* GetModalDialogWebContentsForTesting();
+
+  SigninViewControllerDelegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(SigninViewController);
 };

diff --git a/chrome/browser/ui/signin_view_controller_delegate.h b/chrome/browser/ui/signin_view_controller_delegate.h
@@ -78,7 +78,7 @@ class SigninViewControllerDelegate
 
   // WebContents is used for executing javascript in the context of a modal sync
   // confirmation dialog.
-  content::WebContents* web_contents_for_testing() { return web_contents_; }
+  content::WebContents* web_contents() { return web_contents_; }
 
  protected:
   SigninViewControllerDelegate(SigninViewController* signin_view_controller,

diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc b/chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
@@ -67,7 +67,7 @@ IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest, OnSignInLinkClicked) {
   SignInBrowser(browser());
 
 #if !defined(OS_CHROMEOS)
-  EXPECT_TRUE(browser()->signin_view_controller()->delegate());
+  EXPECT_TRUE(browser()->signin_view_controller()->ShowsModalDialog());
   EXPECT_EQ(starting_tab_count, browser()->tab_strip_model()->count());
 #else
   EXPECT_EQ(starting_tab_count + 1, browser()->tab_strip_model()->count());
@@ -80,7 +80,7 @@ IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest,
   SignInBrowser(browser());
 
 #if !defined(OS_CHROMEOS)
-  EXPECT_TRUE(browser()->signin_view_controller()->delegate());
+  EXPECT_TRUE(browser()->signin_view_controller()->ShowsModalDialog());
 #endif
   EXPECT_EQ(starting_tab_count, browser()->tab_strip_model()->count());
 }
@@ -131,7 +131,7 @@ IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest, BrowserRemoved) {
 
   int tab_count = extra_browser->tab_strip_model()->count();
 #if !defined(OS_CHROMEOS)
-  EXPECT_TRUE(extra_browser->signin_view_controller()->delegate());
+  EXPECT_TRUE(extra_browser->signin_view_controller()->ShowsModalDialog());
   EXPECT_EQ(starting_tab_count, tab_count);
 #else
   // A new tab should have been opened in the extra browser, which should be

diff --git a/chrome/browser/ui/webui/signin/inline_login_handler.cc b/chrome/browser/ui/webui/signin/inline_login_handler.cc
@@ -277,7 +277,7 @@ void InlineLoginHandler::HandleNavigationButtonClicked(
   Browser* browser = signin::GetDesktopBrowser(web_ui());
   DCHECK(browser);
 
-  browser->signin_view_controller()->delegate()->PerformNavigation();
+  browser->signin_view_controller()->PerformNavigation();
 }
 
 void InlineLoginHandler::HandleDialogClose(const base::ListValue* args) {

diff --git a/chrome/browser/ui/webui/signin/login_ui_test_utils.cc b/chrome/browser/ui/webui/signin/login_ui_test_utils.cc
@@ -111,6 +111,35 @@ void RunLoopFor(base::TimeDelta duration) {
 
 
 namespace login_ui_test_utils {
+class SigninViewControllerTestUtil {
+ public:
+  static bool TryDismissSyncConfirmationDialog(Browser* browser) {
+#if defined(OS_CHROMEOS)
+    NOTREACHED();
+    return false;
+#else
+    SigninViewController* signin_view_controller =
+        browser->signin_view_controller();
+    DCHECK_NE(signin_view_controller, nullptr);
+    if (!signin_view_controller->ShowsModalDialog())
+      return false;
+    content::WebContents* dialog_web_contents =
+        signin_view_controller->GetModalDialogWebContentsForTesting();
+    DCHECK_NE(dialog_web_contents, nullptr);
+    std::string message;
+    std::string js =
+        "if (document.getElementById('confirmButton') == null) {"
+        "  window.domAutomationController.send('NotFound');"
+        "} else {"
+        "  document.getElementById('confirmButton').click();"
+        "  window.domAutomationController.send('Ok');"
+        "}";
+    EXPECT_TRUE(content::ExecuteScriptAndExtractString(dialog_web_contents, js,
+                                                       &message));
+    return message == "Ok";
+#endif
+  }
+};
 
 void WaitUntilUIReady(Browser* browser) {
   std::string message;
@@ -252,38 +281,10 @@ bool SignInWithUI(Browser* browser,
                       signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT);
 }
 
-bool TryDismissSyncConfirmationDialog(Browser* browser) {
-#if defined(OS_CHROMEOS)
-  NOTREACHED();
-  return false;
-#else
-  SigninViewController* signin_view_controller =
-      browser->signin_view_controller();
-  DCHECK_NE(signin_view_controller, nullptr);
-  SigninViewControllerDelegate* delegate = signin_view_controller->delegate();
-  if (delegate == nullptr)
-    return false;
-  content::WebContents* dialog_web_contents =
-      delegate->web_contents_for_testing();
-  DCHECK_NE(dialog_web_contents, nullptr);
-  std::string message;
-  std::string js =
-      "if (document.getElementById('confirmButton') == null) {"
-      "  window.domAutomationController.send('NotFound');"
-      "} else {"
-      "  document.getElementById('confirmButton').click();"
-      "  window.domAutomationController.send('Ok');"
-      "}";
-  EXPECT_TRUE(content::ExecuteScriptAndExtractString(dialog_web_contents, js,
-                                                     &message));
-  return message == "Ok";
-#endif
-}
-
 bool DismissSyncConfirmationDialog(Browser* browser, base::TimeDelta timeout) {
   const base::Time expire_time = base::Time::Now() + timeout;
   while (base::Time::Now() <= expire_time) {
-    if (TryDismissSyncConfirmationDialog(browser))
+    if (SigninViewControllerTestUtil::TryDismissSyncConfirmationDialog(browser))
       return true;
     RunLoopFor(base::TimeDelta::FromMilliseconds(1000));
   }