cbui: simplify CredentialLeakDialog lifetime

Dramatis personae:
  CredentialLeakDialogView (CLDV): a DialogDelegateView, owned by Views
  CredentialLeakDialogController (CLDC): an abstract interface used by
    CLDV.
  CredentialLeakDialogControllerImpl (CLDCI): a CLDC instance - actually
    the only one used in production
  ManagePasswordsUIController (MPUIC): a WebContentsUserData that
    controls any password-related UI showing for a given WebContents.
    Owns at most one CLDC instance.

Right now, there are two ways that a CLDV can be closed: in response to
something that happens in the browser, or in response to the user
clicking one of the buttons on the CLDV. The former case looks like,
e.g.:

        MPUIC::OnChooseCredentials
  calls CLDCI::~CLDCI
  calls CLDV::ControllerGone
  calls Widget::Close
  calls NonClientView::CanClose
  calls DialogClientView::CanClose
  calls CLDV::Close
  calls CLDCI::OnCloseDialog
  calls MPUIC::OnLeakDialogHidden

and in MUPIC::OnLeakDialogHidden, dialog_controller_ is already null, so
the stack unwinds.

The *latter* case looks like this though:

        DialogClientView::ButtonPressed
  calls DialogClientView::AcceptWindow
  calls DialogDelegate::Accept [aka CLDV::Accept]
  calls CLDCI::OnAcceptDialog
  calls MPUIC::OnLeakDialogHidden
  calls CLDCI::~CLDCI
  calls CLDCI::ResetDialog
  calls CLDV::ControllerGone
  calls Widget::Close (!)
  calls NonClientView::CanClose
  calls DialogClientView::CanClose
  calls CLDV::Close
  calls CLDCI::OnCloseDialog
  calls MPUIC::OnLeakDialogHidden

at that point in MPUIC::OnLeakDialogHidden, dialog_controller_ is null
because of the previous call on the stack, so the stack unwinds.
However, note that the controller got notified of the closure twice -
once via OnAcceptDialog and once via OnCloseDialog.

To avoid that double notification in case 2, CredentialLeakDialogView
nulls out its own controller in {Accept,Cancel,Close}, with a scary
block comment about why this is done.

The actual issue, though, is the CLDCI::~CLDCI -> CLDV::ControllerGone
-> Widget::Close steps - in case 2, the Widget is *already* in the
process of closing, as soon as {Accept,Cancel,Close} return; it should
not re-enter Widget::Close at all!

In order to avoid that re-entry, this CL has CLDV only close itself if
the controller is still not null in ControllerGone(), so the logic in
the CLDV's accept/cancel/close callbacks that nulls the controller serves
to prevent a recursive close.

This allows dramatically simplifying the logic around closure in this
dialog, and using the "new" closure methods for dialogs.

Bug: 1011446
Change-Id: I60100a31614ba10c5e615b985ff3039a2bfbffe6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028194
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737353}

chrome/browser/ui/views/passwords/credential_leak_dialog_view.cc

@@ -84,6 +84,29 @@ CredentialLeakDialogView::CredentialLeakDialogView(
                                    controller_->GetAcceptButtonLabel());
   DialogDelegate::set_button_label(ui::DIALOG_BUTTON_CANCEL,
                                    controller_->GetCancelButtonLabel());
+
+  using ControllerClosureFn = void (CredentialLeakDialogController::*)(void);
+  auto close_callback = [](CredentialLeakDialogController** controller,
+                           ControllerClosureFn fn) {
+    // Null out the controller pointer stored in the parent object, to avoid any
+    // further calls to the controller and inhibit recursive closes that would
+    // otherwise happen in ControllerGone(), and invoke the provided method on
+    // the controller.
+    //
+    // Note that when this lambda gets bound it closes over &controller_, not
+    // controller_ itself!
+    (std::exchange(*controller, nullptr)->*(fn))();
+  };
+
+  DialogDelegate::set_accept_callback(
+      base::BindOnce(close_callback, base::Unretained(&controller_),
+                     &CredentialLeakDialogController::OnAcceptDialog));
+  DialogDelegate::set_cancel_callback(
+      base::BindOnce(close_callback, base::Unretained(&controller_),
+                     &CredentialLeakDialogController::OnCancelDialog));
+  DialogDelegate::set_close_callback(
+      base::BindOnce(close_callback, base::Unretained(&controller_),
+                     &CredentialLeakDialogController::OnCloseDialog));
 }
 
 CredentialLeakDialogView::~CredentialLeakDialogView() = default;
@@ -95,8 +118,12 @@ void CredentialLeakDialogView::ShowCredentialLeakPrompt() {
 
 void CredentialLeakDialogView::ControllerGone() {
   // Widget::Close() synchronously calls Close() on this instance, which resets
-  // the |controller_|.
-  GetWidget()->Close();
+  // the |controller_|. The null check for |controller_| here is to avoid
+  // reentry into Close() - |controller_| might have been nulled out by the
+  // closure callbacks already, in which case the dialog is already closing. See
+  // the definition of |close_callback| in the constructor.
+  if (controller_)
+    GetWidget()->Close();
 }
 
 ui::ModalType CredentialLeakDialogView::GetModalType() const {
@@ -110,33 +137,6 @@ gfx::Size CredentialLeakDialogView::CalculatePreferredSize() const {
   return gfx::Size(width, GetHeightForWidth(width));
 }
 
-bool CredentialLeakDialogView::Cancel() {
-  if (controller_)
-    // Since OnCancelDialog() synchronously invokes Close() on this instance, we
-    // need to clear the |controller_| before to avoid notifying the controller
-    // twice.
-    std::exchange(controller_, nullptr)->OnCancelDialog();
-  return true;
-}
-
-bool CredentialLeakDialogView::Accept() {
-  if (controller_)
-    // Since OnAcceptDialog() synchronously invokes Close() on this instance, we
-    // need to clear the |controller_| before to avoid notifying the controller
-    // twice.
-    std::exchange(controller_, nullptr)->OnAcceptDialog();
-  return true;
-}
-
-bool CredentialLeakDialogView::Close() {
-  if (controller_)
-    // Since OnCloseDialog() synchronously invokes Close() on this instance, we
-    // need to clear the |controller_| before to avoid notifying the controller
-    // twice.
-    std::exchange(controller_, nullptr)->OnCloseDialog();
-  return true;
-}
-
 int CredentialLeakDialogView::GetDialogButtons() const {
   // |controller_| can be nullptr when the framework calls this method after a
   // button click.

chrome/browser/ui/views/passwords/credential_leak_dialog_view.h

@@ -30,9 +30,6 @@ class CredentialLeakDialogView : public views::DialogDelegateView,
   // views::DialogDelegateView:
   ui::ModalType GetModalType() const override;
   gfx::Size CalculatePreferredSize() const override;
-  bool Cancel() override;
-  bool Accept() override;
-  bool Close() override;
   int GetDialogButtons() const override;
   bool ShouldShowCloseButton() const override;
   void OnThemeChanged() override;