[Autofill Auth UI] Fix crashing if closing browser when dialog is visible

Did some manual testings to make sure it is working correctly.

Bug: 991037
Change-Id: Icceddefdee99f0ba252f5ce161e4b42813ee1c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1848751Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Commit-Queue: Siyu An <siyua@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704886}

chrome/browser/ui/autofill/payments/webauthn_offer_dialog_browsertest.cc

-// Copyright 2019 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.
-
-#include "chrome/browser/ui/autofill/payments/webauthn_offer_dialog_controller_impl.h"
-#include "chrome/browser/ui/autofill/payments/webauthn_offer_dialog_view.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/test/test_browser_dialog.h"
-
-namespace autofill {
-
-class WebauthnOfferDialogBrowsertest : public DialogBrowserTest {
- public:
-  WebauthnOfferDialogBrowsertest() = default;
-
-  // DialogBrowserTest:
-  void ShowUi(const std::string& name) override {
-    content::WebContents* web_contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
-
-    // Do lazy initialization of WebauthnOfferDialogControllerImpl.
-    WebauthnOfferDialogControllerImpl::CreateForWebContents(web_contents);
-    WebauthnOfferDialogControllerImpl* controller =
-        WebauthnOfferDialogControllerImpl::FromWebContents(web_contents);
-    DCHECK(controller);
-    controller->ShowOfferDialog(base::DoNothing());
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WebauthnOfferDialogBrowsertest);
-};
-
-IN_PROC_BROWSER_TEST_F(WebauthnOfferDialogBrowsertest, InvokeUi_default) {
-  ShowAndVerifyUi();
-}
-
-}  // namespace autofill

chrome/browser/ui/autofill/payments/webauthn_offer_dialog_controller_impl.cc

@@ -13,15 +13,25 @@ WebauthnOfferDialogControllerImpl::WebauthnOfferDialogControllerImpl(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents) {}
 
-WebauthnOfferDialogControllerImpl::~WebauthnOfferDialogControllerImpl() =
-    default;
+WebauthnOfferDialogControllerImpl::~WebauthnOfferDialogControllerImpl() {
+  // This part of code is executed only if browser window is closed when the
+  // dialog is visible. In this case the controller is destroyed before
+  // WebauthnOfferDialogViewImpl::dtor() being called, but the reference to
+  // controller is not reset. Need to reset via
+  // WebauthnOfferDialogViewImpl::Hide() to avoid crash.
+  if (dialog_model_) {
+    dialog_model_->SetDialogState(
+        WebauthnOfferDialogModel::DialogState::kInactive);
+  }
+}
 
 void WebauthnOfferDialogControllerImpl::ShowOfferDialog(
     AutofillClient::WebauthnOfferDialogCallback callback) {
   DCHECK(!dialog_model_);
 
   offer_dialog_callback_ = std::move(callback);
-  dialog_model_ = WebauthnOfferDialogView::CreateAndShow(this);
+  dialog_view_ = WebauthnOfferDialogView::CreateAndShow(this);
+  dialog_model_ = dialog_view_->GetDialogModel();
 }
 
 bool WebauthnOfferDialogControllerImpl::CloseDialog() {
@@ -40,6 +50,7 @@ void WebauthnOfferDialogControllerImpl::UpdateDialogWithError() {
 
 void WebauthnOfferDialogControllerImpl::OnDialogClosed() {
   dialog_model_ = nullptr;
+  dialog_view_ = nullptr;
   offer_dialog_callback_.Reset();
 }
 

chrome/browser/ui/autofill/payments/webauthn_offer_dialog_controller_impl.h

@@ -14,6 +14,7 @@
 namespace autofill {
 
 class WebauthnOfferDialogModel;
+class WebauthnOfferDialogView;
 
 // Implementation of the per-tab controller to control the
 // WebauthnOfferDialogView. Lazily initialized when used.
@@ -34,6 +35,8 @@ class WebauthnOfferDialogControllerImpl
   void OnDialogClosed() override;
   content::WebContents* GetWebContents() override;
 
+  WebauthnOfferDialogView* dialog_view() { return dialog_view_; }
+
  protected:
   explicit WebauthnOfferDialogControllerImpl(
       content::WebContents* web_contents);
@@ -47,6 +50,7 @@ class WebauthnOfferDialogControllerImpl
   AutofillClient::WebauthnOfferDialogCallback offer_dialog_callback_;
 
   WebauthnOfferDialogModel* dialog_model_ = nullptr;
+  WebauthnOfferDialogView* dialog_view_ = nullptr;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

chrome/browser/ui/autofill/payments/webauthn_offer_dialog_view.h

@@ -15,8 +15,10 @@ class WebauthnOfferDialogModel;
 // model of the dialog.
 class WebauthnOfferDialogView {
  public:
-  static WebauthnOfferDialogModel* CreateAndShow(
+  static WebauthnOfferDialogView* CreateAndShow(
       WebauthnOfferDialogController* controller);
+
+  virtual WebauthnOfferDialogModel* GetDialogModel() const = 0;
 };
 
 }  // namespace autofill

chrome/browser/ui/views/autofill/payments/webauthn_offer_dialog_browsertest.cc

+// Copyright 2019 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.
+
+#include "base/run_loop.h"
+#include "chrome/browser/ui/autofill/payments/webauthn_offer_dialog_controller_impl.h"
+#include "chrome/browser/ui/autofill/payments/webauthn_offer_dialog_view.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/test/test_browser_dialog.h"
+#include "chrome/browser/ui/views/autofill/payments/webauthn_offer_dialog_view_impl.h"
+#include "ui/views/window/dialog_client_view.h"
+
+namespace autofill {
+
+class WebauthnOfferDialogBrowserTest : public DialogBrowserTest {
+ public:
+  WebauthnOfferDialogBrowserTest() = default;
+
+  // DialogBrowserTest:
+  void ShowUi(const std::string& name) override {
+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+
+    // Do lazy initialization of WebauthnOfferDialogControllerImpl.
+    WebauthnOfferDialogControllerImpl::CreateForWebContents(web_contents);
+    DCHECK(controller());
+    controller()->ShowOfferDialog(base::DoNothing());
+  }
+
+  WebauthnOfferDialogViewImpl* GetWebauthnOfferDialog() {
+    if (!controller())
+      return nullptr;
+
+    WebauthnOfferDialogView* dialog_view = controller()->dialog_view();
+    if (!dialog_view)
+      return nullptr;
+
+    return static_cast<WebauthnOfferDialogViewImpl*>(dialog_view);
+  }
+
+  WebauthnOfferDialogControllerImpl* controller() {
+    if (!browser() || !browser()->tab_strip_model() ||
+        !browser()->tab_strip_model()->GetActiveWebContents())
+      return nullptr;
+
+    return WebauthnOfferDialogControllerImpl::FromWebContents(
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WebauthnOfferDialogBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(WebauthnOfferDialogBrowserTest, InvokeUi_default) {
+  ShowAndVerifyUi();
+}
+
+// Ensures closing tab while dialog being visible is correctly handled.
+// RunUntilIdle() makes sure that nothing crashes after browser tab is closed.
+IN_PROC_BROWSER_TEST_F(WebauthnOfferDialogBrowserTest,
+                       CanCloseTabWhileDialogShowing) {
+  ShowUi(std::string());
+  VerifyUi();
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+  base::RunLoop().RunUntilIdle();
+}
+
+// Ensures closing browser while dialog being visible is correctly handled.
+IN_PROC_BROWSER_TEST_F(WebauthnOfferDialogBrowserTest,
+                       CanCloseBrowserWhileDialogShowing) {
+  ShowUi(std::string());
+  VerifyUi();
+  browser()->window()->Close();
+  base::RunLoop().RunUntilIdle();
+}
+
+// Ensures dialog is closed when cancel button is clicked.
+IN_PROC_BROWSER_TEST_F(WebauthnOfferDialogBrowserTest, ClickCancelButton) {
+  ShowUi(std::string());
+  VerifyUi();
+  GetWebauthnOfferDialog()->GetDialogClientView()->CancelWindow();
+  base::RunLoop().RunUntilIdle();
+}
+
+}  // namespace autofill

chrome/browser/ui/views/autofill/payments/webauthn_offer_dialog_view_impl.cc

@@ -34,16 +34,24 @@ WebauthnOfferDialogViewImpl::WebauthnOfferDialogViewImpl(
 
 WebauthnOfferDialogViewImpl::~WebauthnOfferDialogViewImpl() {
   model_->RemoveObserver(this);
+  if (controller_) {
+    controller_->OnDialogClosed();
+    controller_ = nullptr;
+  }
 }
 
 // static
-WebauthnOfferDialogModel* WebauthnOfferDialogView::CreateAndShow(
+WebauthnOfferDialogView* WebauthnOfferDialogView::CreateAndShow(
     WebauthnOfferDialogController* controller) {
   WebauthnOfferDialogViewImpl* dialog =
       new WebauthnOfferDialogViewImpl(controller);
   constrained_window::ShowWebModalDialogViews(dialog,
                                               controller->GetWebContents());
-  return dialog->model();
+  return dialog;
+}
+
+WebauthnOfferDialogModel* WebauthnOfferDialogViewImpl::GetDialogModel() const {
+  return model_;
 }
 
 void WebauthnOfferDialogViewImpl::OnDialogStateChanged() {
@@ -124,14 +132,14 @@ bool WebauthnOfferDialogViewImpl::ShouldShowCloseButton() const {
   return false;
 }
 
-void WebauthnOfferDialogViewImpl::WindowClosing() {
+void WebauthnOfferDialogViewImpl::Hide() {
+  // Reset controller reference if the controller has been destroyed before the
+  // view being destroyed. This happens if browser window is closed when the
+  // dialog is visible.
   if (controller_) {
     controller_->OnDialogClosed();
     controller_ = nullptr;
   }
-}
-
-void WebauthnOfferDialogViewImpl::Hide() {
   GetWidget()->Close();
 }
 

chrome/browser/ui/views/autofill/payments/webauthn_offer_dialog_view_impl.h

@@ -28,6 +28,9 @@ class WebauthnOfferDialogViewImpl : public WebauthnOfferDialogView,
       WebauthnOfferDialogController* controller);
   ~WebauthnOfferDialogViewImpl() override;
 
+  // WebauthnOfferDialogView:
+  WebauthnOfferDialogModel* GetDialogModel() const override;
+
   // WebauthnOfferDialogModelObserver:
   void OnDialogStateChanged() override;
 
@@ -43,9 +46,7 @@ class WebauthnOfferDialogViewImpl : public WebauthnOfferDialogView,
   base::string16 GetWindowTitle() const override;
   bool ShouldShowWindowTitle() const override;
   bool ShouldShowCloseButton() const override;
-  void WindowClosing() override;
 
-  WebauthnOfferDialogModel* model() { return model_; }
 
  private:
   // Closes the dialog.

chrome/test/BUILD.gn

@@ -1193,7 +1193,6 @@ if (!is_android) {
       "../browser/ui/autofill/payments/card_unmask_prompt_view_browsertest.cc",
       "../browser/ui/autofill/payments/card_unmask_prompt_view_tester.h",
       "../browser/ui/autofill/payments/save_card_bubble_controller_impl_browsertest.cc",
-      "../browser/ui/autofill/payments/webauthn_offer_dialog_browsertest.cc",
       "../browser/ui/bookmarks/bookmark_browsertest.cc",
       "../browser/ui/browser_browsertest.cc",
       "../browser/ui/browser_command_controller_browsertest.cc",
@@ -1850,6 +1849,7 @@ if (!is_android) {
         "../browser/ui/views/autofill/payments/card_unmask_prompt_view_tester_views.h",
         "../browser/ui/views/autofill/payments/local_card_migration_browsertest.cc",
         "../browser/ui/views/autofill/payments/save_card_bubble_views_browsertest.cc",
+        "../browser/ui/views/autofill/payments/webauthn_offer_dialog_browsertest.cc",
         "../browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc",
         "../browser/ui/views/bookmarks/bookmark_editor_view_browsertest.cc",
         "../browser/ui/views/certificate_selector_dialog_browsertest.cc",