Split the dialog in LoginHandlerViews into its own object.

views wants to (more or less) own the DialogDelegate. This is currently
done via refcounting. To remove that dependency of the refcounts,
extract it into its own object, with lifetime contracts against
LoginHandler.

Now the threading and refcounting mess is largely confined to the
LoginHandler base class, rather than spread into the subclasses.

Bug: 908926
Change-Id: Ia7f88c23d051dff06492baee2b050f82265a59e3
Reviewed-on: https://chromium-review.googlesource.com/c/1388037Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626194}

chrome/browser/ui/views/login_handler_views.cc

@@ -18,14 +18,15 @@
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
-// ----------------------------------------------------------------------------
-// LoginHandlerViews
+namespace chrome {
+
+namespace {
 
 // This class simply forwards the authentication from the LoginView (on
 // the UI thread) to the net::URLRequest (on the I/O thread).
 // This class uses ref counting to ensure that it lives until all InvokeLaters
 // have been called.
-class LoginHandlerViews : public LoginHandler, public views::DialogDelegate {
+class LoginHandlerViews : public LoginHandler {
  public:
   LoginHandlerViews(
       net::AuthChallengeInfo* auth_info,
@@ -33,67 +34,8 @@ class LoginHandlerViews : public LoginHandler, public views::DialogDelegate {
       LoginAuthRequiredCallback auth_required_callback)
       : LoginHandler(auth_info,
                      web_contents_getter,
-                     std::move(auth_required_callback)),
-        login_view_(nullptr),
-        dialog_(nullptr) {
-    chrome::RecordDialogCreation(chrome::DialogIdentifier::LOGIN_HANDLER);
-  }
-
-  // views::DialogDelegate:
-  bool ShouldShowCloseButton() const override { return false; }
-
-  base::string16 GetDialogButtonLabel(ui::DialogButton button) const override {
-    if (button == ui::DIALOG_BUTTON_OK)
-      return l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_OK_BUTTON_LABEL);
-    return DialogDelegate::GetDialogButtonLabel(button);
-  }
-
-  base::string16 GetWindowTitle() const override {
-    return l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_TITLE);
-  }
-
-  void WindowClosing() override {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-    // Reference is no longer valid.
-    dialog_ = NULL;
-    CancelAuth();
-  }
-
-  void DeleteDelegate() override {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-    // The widget is going to delete itself; clear our pointer.
-    dialog_ = NULL;
-
-    // This Release is the counter-point to the AddRef() in BuildViewImpl().
-    Release();
-
-    ReleaseSoon();
-  }
-
-  ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; }
-
-  bool Cancel() override {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    CancelAuth();
-    return true;
-  }
-
-  bool Accept() override {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    SetAuth(login_view_->GetUsername(), login_view_->GetPassword());
-    return true;
-  }
-
-  views::View* GetInitiallyFocusedView() override {
-    return login_view_->GetInitiallyFocusedView();
-  }
-
-  views::View* GetContentsView() override { return login_view_; }
-  views::Widget* GetWidget() override { return login_view_->GetWidget(); }
-  const views::Widget* GetWidget() const override {
-    return login_view_->GetWidget();
+                     std::move(auth_required_callback)) {
+    RecordDialogCreation(DialogIdentifier::LOGIN_HANDLER);
   }
 
   // LoginHandler:
@@ -101,50 +43,134 @@ class LoginHandlerViews : public LoginHandler, public views::DialogDelegate {
                      const base::string16& explanation,
                      LoginModelData* login_model_data) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    DCHECK(!dialog_);
 
-    // Create a new LoginView and set the model for it.  The model (password
-    // manager) is owned by the WebContents, but the view is parented to the
-    // browser window, so the view may be destroyed after the password
-    // manager. The view listens for model destruction and unobserves
-    // accordingly.
-    login_view_ = new LoginView(authority, explanation, login_model_data);
-
-    // Views requires the WidgetDelegate [this instance] live longer than the
-    // Widget. To enforce this, we AddRef() here and Release() in
-    // DeleteDelegate().
-    AddRef();
-
-    // Scary thread safety note: This can potentially be called *after* SetAuth
-    // or CancelAuth (say, if the request was cancelled before the UI thread got
-    // control).  However, that's OK since any UI interaction in those functions
-    // will occur via an InvokeLater on the UI thread, which is guaranteed
-    // to happen after this is called (since this was InvokeLater'd first).
-    dialog_ = constrained_window::ShowWebModalDialogViews(
-        this, GetWebContentsForLogin());
+    dialog_ = new Dialog(this, GetWebContentsForLogin(), authority, explanation,
+                         login_model_data);
     NotifyAuthNeeded();
   }
 
   void CloseDialog() override {
     // The hosting widget may have been freed.
-    if (dialog_)
-      dialog_->Close();
+    if (dialog_) {
+      dialog_->CloseDialog();
+      dialog_ = nullptr;
+    }
   }
 
  private:
   friend class base::RefCountedThreadSafe<LoginHandlerViews>;
-  friend class LoginPrompt;
 
-  ~LoginHandlerViews() override {}
-
-  // The LoginView that contains the user's login information.
-  LoginView* login_view_;
-
-  views::Widget* dialog_;
+  ~LoginHandlerViews() override = default;
+
+  void OnDialogDestroyed() { dialog_ = nullptr; }
+
+  // The DialogDelegate is a separate object from LoginHandlerViews so it can be
+  // owned by the views hierarchy (see DeleteDelegate).
+  class Dialog : public views::DialogDelegate {
+   public:
+    // Creates a Dialog which reports the results back to |handler|. Note the
+    // Dialog is responsible for its own lifetime, which may be independent of
+    // |handler|. |handler| may decide to close the Dialog, by calling
+    // CloseDialog, or the Dialog may have been destroyed by the views
+    // hierarchy, in which case it will call handler->OnDialogDestroyed. When
+    // one of these methods is called, whichever comes first, each object must
+    // release pointers to the other.
+    Dialog(LoginHandlerViews* handler,
+           content::WebContents* web_contents,
+           const base::string16& authority,
+           const base::string16& explanation,
+           LoginHandler::LoginModelData* login_model_data)
+        : handler_(handler), login_view_(nullptr), widget_(nullptr) {
+      // Create a new LoginView and set the model for it.  The model (password
+      // manager) is owned by the WebContents, but the view is parented to the
+      // browser window, so the view may be destroyed after the password
+      // manager. The view listens for model destruction and unobserves
+      // accordingly.
+      login_view_ = new LoginView(authority, explanation, login_model_data);
+
+      // ShowWebModalDialogViews takes ownership of this, by way of the
+      // DeleteDelegate method.
+      widget_ = constrained_window::ShowWebModalDialogViews(this, web_contents);
+    }
+
+    void CloseDialog() {
+      handler_ = nullptr;
+      // The hosting widget may have been freed.
+      if (widget_)
+        widget_->Close();
+    }
+
+    // views::DialogDelegate:
+    bool ShouldShowCloseButton() const override { return false; }
+
+    base::string16 GetDialogButtonLabel(
+        ui::DialogButton button) const override {
+      if (button == ui::DIALOG_BUTTON_OK)
+        return l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_OK_BUTTON_LABEL);
+      return DialogDelegate::GetDialogButtonLabel(button);
+    }
+
+    base::string16 GetWindowTitle() const override {
+      return l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_TITLE);
+    }
+
+    void WindowClosing() override {
+      // Reference is no longer valid.
+      widget_ = nullptr;
+      if (handler_)
+        handler_->CancelAuth();
+    }
+
+    void DeleteDelegate() override { delete this; }
+
+    ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; }
+
+    bool Cancel() override {
+      DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+      if (handler_)
+        handler_->CancelAuth();
+      return true;
+    }
+
+    bool Accept() override {
+      DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+      if (handler_)
+        handler_->SetAuth(login_view_->GetUsername(),
+                          login_view_->GetPassword());
+      return true;
+    }
+
+    views::View* GetInitiallyFocusedView() override {
+      return login_view_->GetInitiallyFocusedView();
+    }
+
+    views::View* GetContentsView() override { return login_view_; }
+    views::Widget* GetWidget() override { return login_view_->GetWidget(); }
+    const views::Widget* GetWidget() const override {
+      return login_view_->GetWidget();
+    }
+
+   private:
+    ~Dialog() override {
+      if (handler_)
+        handler_->OnDialogDestroyed();
+    }
+
+    LoginHandlerViews* handler_;
+    // The LoginView that contains the user's login information.
+    LoginView* login_view_;
+    views::Widget* widget_;
+
+    DISALLOW_COPY_AND_ASSIGN(Dialog);
+  };
+
+  Dialog* dialog_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(LoginHandlerViews);
 };
 
-namespace chrome {
+}  // namespace
 
 scoped_refptr<LoginHandler> CreateLoginHandlerViews(
     net::AuthChallengeInfo* auth_info,