Make BubbleDialogModelHost::Close as-if synchronous

This tears down DialogModel by through model_.reset() to simulate dialog
closing being synchronous. This gives simpler lifetimes for client code
as a synchronous delete is more predictable than an asynchronous task
from GetWidget()->Close() on its own which is asynchronous.

Note that from an implementation perspective the Widget teardown is not
synchronous, but this should not be a concern of DialogModel client
code.

Bug: 1106422
Change-Id: Ib35ec2389e78459ae98a4f901a35858a7fe97189
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2372929Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801470}

ui/base/models/dialog_model_host.h

@@ -14,9 +14,9 @@ class DialogModel;
 // Platform-agnostic interface for toolkit integrations.
 class COMPONENT_EXPORT(UI_BASE) DialogModelHost {
  public:
-  // TODO(pbos): Try to get Close semantically synchronous (guarantee
-  // synchronous destruction of model), so it cannot be observed as
-  // asynchronous even if GetWidget()->Close() under Views is async.
+  // Immediately closes the DialogModelHost. Calling Close() destroys the
+  // DialogModel and no subsequent calls should be made into either DialogModel
+  // or DialogModelHost.
   virtual void Close() = 0;
 
   // Selects all text of a textfield.

ui/views/BUILD.gn

@@ -1045,6 +1045,7 @@ test("views_unittests") {
     "border_unittest.cc",
     "bubble/bubble_border_unittest.cc",
     "bubble/bubble_dialog_delegate_view_unittest.cc",
+    "bubble/bubble_dialog_model_host_unittest.cc",
     "bubble/bubble_frame_view_unittest.cc",
     "bubble/info_bubble_unittest.cc",
     "controls/base_control_test_widget.cc",

ui/views/bubble/bubble_dialog_model_host.cc

@@ -48,6 +48,9 @@ BubbleDialogModelHost::BubbleDialogModelHost(
 
   ConfigureGridLayout();
 
+  // Dialog callbacks can safely refer to |model_|, they can't be called after
+  // Widget::Close() calls WidgetWillClose() synchronously so there shouldn't
+  // be any dangling references after model removal.
   SetAcceptCallback(base::BindOnce(&ui::DialogModel::OnDialogAccepted,
                                    base::Unretained(model_.get()),
                                    GetPassKey()));
@@ -57,9 +60,13 @@ BubbleDialogModelHost::BubbleDialogModelHost(
   SetCloseCallback(base::BindOnce(&ui::DialogModel::OnDialogClosed,
                                   base::Unretained(model_.get()),
                                   GetPassKey()));
-  RegisterWindowClosingCallback(
-      base::BindOnce(&ui::DialogModel::OnWindowClosing,
-                     base::Unretained(model_.get()), GetPassKey()));
+
+  // WindowClosingCallback happens on native widget destruction which is after
+  // |model_| reset. Hence routing this callback through |this| so that we only
+  // forward the call to DialogModel::OnWindowClosing if we haven't already been
+  // closed.
+  RegisterWindowClosingCallback(base::BindOnce(
+      &BubbleDialogModelHost::OnWindowClosing, base::Unretained(this)));
 
   int button_mask = ui::DIALOG_BUTTON_NONE;
   auto* ok_button = model_->ok_button(GetPassKey());
@@ -127,9 +134,21 @@ gfx::Size BubbleDialogModelHost::CalculatePreferredSize() const {
 }
 
 void BubbleDialogModelHost::Close() {
-  // TODO(pbos): Synchronously destroy model here, as-if closing immediately.
+  DCHECK(model_);
   DCHECK(GetWidget());
   GetWidget()->Close();
+
+  // Synchronously destroy |model_|. Widget::Close() being asynchronous should
+  // not be observable by the model.
+
+  // Notify the model of window closing before destroying it (as if
+  // Widget::Close)
+  model_->OnWindowClosing(GetPassKey());
+
+  // TODO(pbos): Consider turning this into for-each-field remove field.
+  RemoveAllChildViews(true);
+  view_to_field_.clear();
+  model_.reset();
 }
 
 void BubbleDialogModelHost::SelectAllText(int unique_id) {
@@ -192,6 +211,14 @@ void BubbleDialogModelHost::AddInitialFields() {
       first_field_content_type, last_field_content_type));
 }
 
+void BubbleDialogModelHost::OnWindowClosing() {
+  // If the model has been removed we have already notified it of closing on the
+  // ::Close() stack.
+  if (!model_)
+    return;
+  model_->OnWindowClosing(GetPassKey());
+}
+
 GridLayout* BubbleDialogModelHost::GetGridLayout() {
   return static_cast<GridLayout*>(GetLayoutManager());
 }

ui/views/bubble/bubble_dialog_model_host.h

@@ -24,7 +24,7 @@ class Textfield;
 // hosts a ui::DialogModel as a BubbleDialogDelegateView. This exposes
 // views-specific methods such as SetAnchorView(), SetArrow() and
 // SetHighlightedButton(). For methods that are reflected in ui::DialogModelHost
-// (such as ::Close()), preferusing the ui::DialogModelHost to avoid
+// (such as ::Close()), prefer using the ui::DialogModelHost to avoid
 // platform-specific code (GetWidget()->Close()) where unnecessary. For those
 // methods, note that this can be retrieved as a ui::DialogModelHost through
 // DialogModel::host(). This helps minimize platform-specific code from
@@ -60,6 +60,8 @@ class VIEWS_EXPORT BubbleDialogModelHost : public BubbleDialogDelegateView,
   void OnPerformAction(views::Combobox* combobox) override;
 
  private:
+  void OnWindowClosing();
+
   GridLayout* GetGridLayout();
   void ConfigureGridLayout();
 

ui/views/bubble/bubble_dialog_model_host_unittest.cc

+// Copyright 2020 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 "ui/views/bubble/bubble_dialog_model_host.h"
+
+#include <memory>
+#include <utility>
+
+#include "ui/views/test/views_test_base.h"
+#include "ui/views/test/widget_test.h"
+
+namespace views {
+
+using BubbleDialogModelHostTest = ViewsTestBase;
+
+// TODO(pbos): Consider moving tests from this file into a test base for
+// DialogModel that can be instantiated by any DialogModelHost implementation to
+// check its compliance.
+
+namespace {
+// TODO(pbos): Consider moving this to a non-views testutil location. This is
+// likely usable without/outside views (even if the test suite doesn't move).
+class TestModelDelegate : public ui::DialogModelDelegate {
+ public:
+  struct Stats {
+    int window_closing_count = 0;
+  };
+
+  explicit TestModelDelegate(Stats* stats) : stats_(stats) {}
+
+  base::WeakPtr<TestModelDelegate> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+  static std::unique_ptr<ui::DialogModel> BuildModel(
+      std::unique_ptr<TestModelDelegate> delegate) {
+    auto* delegate_ptr = delegate.get();
+    return ui::DialogModel::Builder(std::move(delegate))
+        .SetWindowClosingCallback(
+            base::BindOnce(&TestModelDelegate::OnWindowClosing,
+                           base::Unretained(delegate_ptr)))
+        .Build();
+  }
+
+  void OnWindowClosing() { ++stats_->window_closing_count; }
+
+ private:
+  Stats* const stats_;
+  base::WeakPtrFactory<TestModelDelegate> weak_ptr_factory_{this};
+};
+}  // namespace
+
+TEST_F(BubbleDialogModelHostTest, CloseIsSynchronousAndCallsWindowClosing) {
+  std::unique_ptr<Widget> anchor_widget =
+      CreateTestWidget(Widget::InitParams::TYPE_WINDOW);
+
+  TestModelDelegate::Stats stats;
+  auto delegate = std::make_unique<TestModelDelegate>(&stats);
+  auto weak_delegate = delegate->GetWeakPtr();
+
+  auto host = std::make_unique<views::BubbleDialogModelHost>(
+      TestModelDelegate::BuildModel(std::move(delegate)));
+  host->SetAnchorView(anchor_widget->GetContentsView());
+  auto* host_ptr = host.get();
+
+  Widget* bubble_widget =
+      BubbleDialogDelegateView::CreateBubble(host.release());
+  test::WidgetDestroyedWaiter waiter(bubble_widget);
+
+  EXPECT_EQ(0, stats.window_closing_count);
+  DCHECK_EQ(host_ptr, weak_delegate->dialog_model()->host());
+  weak_delegate->dialog_model()->host()->Close();
+  EXPECT_EQ(1, stats.window_closing_count);
+
+  // The model (and hence delegate) should destroy synchronously, so the
+  // WeakPtr should disappear before waiting for the views Widget to close.
+  EXPECT_FALSE(weak_delegate);
+
+  waiter.Wait();
+}
+
+}  // namespace views