views: never deliver multiple dialog closure callbacks

Currently, if a DialogDelegate's Accept() or Cancel() method leads back
into its Close(), two of the registered callbacks can be run, which is a
violation of DialogDelegate's contract and causes confusing workarounds
in client code. This often happens in code like this:

void SomeBubble::OnDialogAccepted() {
  NavigateTo(kSomeURL);
}

where NavigateTo causes a page navigation synchronously, which causes
the involved bubble to be closed.

Instead:

* In the near term, DialogDelegate should ensure that it never delivers
  more than one of these callbacks
* In the far term, DialogDelegate should enforce that callers do not
  re-enter Close() from inside Accept() or Cancel()

The latter will likely need to wait for the Accept/Cancel/Close callback
refactor to be finished.

This change also removes the remnants of the GlobalErrorBubbleView unit test
suite, which were exercising this behavior deliberately. These tests
were providing very little actual coverage and I have been gradually
deleting them as I refactor GlobalError anyway.

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

chrome/browser/ui/views/global_error_bubble_view_unittest.cc

-// Copyright 2017 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/views/global_error_bubble_view.h"
-
-#include <memory>
-#include <vector>
-
-#include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/ui/global_error/global_error.h"
-#include "chrome/browser/ui/views/chrome_layout_provider.h"
-#include "chrome/test/views/chrome_test_views_delegate.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "ui/gfx/codec/png_codec.h"
-#include "ui/gfx/image/image.h"
-#include "ui/gfx/image/image_png_rep.h"
-#include "ui/gfx/image/image_unittest_util.h"
-#include "ui/views/controls/button/button.h"
-#include "ui/views/controls/button/label_button.h"
-
-using ::testing::_;
-using ::testing::Return;
-using ::testing::StrictMock;
-
-namespace views {
-
-namespace {
-
-class MockGlobalErrorWithStandardBubble : public GlobalErrorWithStandardBubble {
- public:
-  MockGlobalErrorWithStandardBubble() {}
-  ~MockGlobalErrorWithStandardBubble() override;
-
-  MOCK_METHOD0(GetBubbleViewIcon, gfx::Image());
-  MOCK_METHOD0(GetBubbleViewTitle, base::string16());
-  MOCK_METHOD0(GetBubbleViewMessage, std::vector<base::string16>());
-  MOCK_CONST_METHOD0(ShouldShowCloseButton, bool());
-  MOCK_METHOD0(ShouldAddElevationIconToAcceptButton, bool());
-  MOCK_METHOD1(BubbleViewDidClose, void(Browser* browser));
-  MOCK_METHOD1(OnBubbleViewDidClose, void(Browser* browser));
-  MOCK_METHOD1(BubbleViewAcceptButtonPressed, void(Browser* browser));
-  MOCK_METHOD1(BubbleViewCancelButtonPressed, void(Browser* browser));
-  MOCK_CONST_METHOD0(ShouldCloseOnDeactivate, bool());
-  MOCK_METHOD0(HasBubbleView, bool());
-  MOCK_METHOD0(HasShownBubbleView, bool());
-  MOCK_METHOD1(ShowBubbleView, void(Browser* browser));
-  MOCK_METHOD0(GetBubbleView, GlobalErrorBubbleViewBase*());
-  MOCK_METHOD0(HasMenuItem, bool());
-  MOCK_METHOD0(MenuItemCommandID, int());
-  MOCK_METHOD0(MenuItemLabel, base::string16());
-  MOCK_METHOD1(ExecuteMenuItem, void(Browser* browser));
-  MOCK_METHOD0(GetBubbleViewMessages, std::vector<base::string16>());
-
-  base::string16 GetBubbleViewAcceptButtonLabel() {
-    return base::UTF8ToUTF16("Ok");
-  }
-  base::string16 GetBubbleViewCancelButtonLabel() {
-    return base::UTF8ToUTF16("Cancel");
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockGlobalErrorWithStandardBubble);
-};
-
-MockGlobalErrorWithStandardBubble::~MockGlobalErrorWithStandardBubble() {}
-
-}  // namespace
-
-class GlobalErrorBubbleViewTest : public testing::Test {
- public:
-  GlobalErrorBubbleViewTest()
-      : mock_global_error_with_standard_bubble_(
-            std::make_unique<StrictMock<MockGlobalErrorWithStandardBubble>>()),
-        button_(nullptr, base::string16()),
-        view_(std::make_unique<GlobalErrorBubbleView>(
-            &arg_view_,
-            gfx::Rect(gfx::Point(), gfx::Size()),
-            views::BubbleBorder::NONE,
-            nullptr,
-            mock_global_error_with_standard_bubble_->AsWeakPtr())) {}
-
- protected:
-  ChromeTestViewsDelegate test_views_delegate_;
-  std::unique_ptr<StrictMock<MockGlobalErrorWithStandardBubble>>
-      mock_global_error_with_standard_bubble_;
-  views::View arg_view_;
-  views::LabelButton button_;
-  std::unique_ptr<GlobalErrorBubbleView> view_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(GlobalErrorBubbleViewTest);
-};
-
-TEST_F(GlobalErrorBubbleViewTest, Basic) {
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_, GetBubbleViewTitle());
-  view_->GetWindowTitle();
-
-  std::vector<gfx::ImagePNGRep> image_png_reps;
-  image_png_reps.push_back(
-      gfx::ImagePNGRep(gfx::test::CreatePNGBytes(25), 1.0f));
-  gfx::Image image = gfx::Image(image_png_reps);
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_, GetBubbleViewIcon())
-      .WillOnce(Return(image));
-  view_->GetWindowIcon();
-
-  EXPECT_EQ(ChromeLayoutProvider::Get()->ShouldShowWindowIcon(),
-            view_->ShouldShowWindowIcon());
-
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              BubbleViewDidClose(nullptr));
-  view_->WindowClosing();
-
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              ShouldShowCloseButton());
-  view_->ShouldShowCloseButton();
-  view_->GetDialogButtons();
-
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              BubbleViewCancelButtonPressed(nullptr));
-  view_->Cancel();
-
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              BubbleViewAcceptButtonPressed(nullptr));
-  view_->Accept();
-}
-
-TEST_F(GlobalErrorBubbleViewTest, ErrorIsNull) {
-  mock_global_error_with_standard_bubble_.reset();
-  view_->GetWindowTitle();
-  view_->WindowClosing();
-
-  view_->ShouldShowCloseButton();
-
-  view_->Cancel();
-  view_->Accept();
-}
-
-}  // namespace views

chrome/test/BUILD.gn

@@ -5326,7 +5326,6 @@ test("unit_tests") {
       "../browser/ui/views/frame/test_with_browser_view.h",
       "../browser/ui/views/frame/web_contents_close_handler_unittest.cc",
       "../browser/ui/views/fullscreen_control/fullscreen_control_popup_unittest.cc",
-      "../browser/ui/views/global_error_bubble_view_unittest.cc",
       "../browser/ui/views/global_media_controls/media_notification_container_impl_view_unittest.cc",
       "../browser/ui/views/global_media_controls/media_notification_list_view_unittest.cc",
       "../browser/ui/views/hover_button_unittest.cc",

ui/views/window/dialog_delegate.cc

@@ -142,20 +142,24 @@ bool DialogDelegate::IsDialogButtonEnabled(ui::DialogButton button) const {
 
 bool DialogDelegate::Cancel() {
   if (cancel_callback_)
-    std::move(cancel_callback_).Run();
+    RunCloseCallback(std::move(cancel_callback_));
   return true;
 }
 
 bool DialogDelegate::Accept() {
   if (accept_callback_)
-    std::move(accept_callback_).Run();
+    RunCloseCallback(std::move(accept_callback_));
   return true;
 }
 
 bool DialogDelegate::Close() {
-  if (close_callback_ || cancel_callback_ || accept_callback_) {
+  // Test for callback_delivered_ here, because it indicates that:
+  // 1) A new-style callback used to be present in one of these slots
+  // 2) It has already been invoked and therefore the slot is now empty
+  if (callback_delivered_ || close_callback_ || cancel_callback_ ||
+      accept_callback_) {
     if (close_callback_)
-      std::move(close_callback_).Run();
+      RunCloseCallback(std::move(close_callback_));
     return true;
   } else {
     return DefaultClose();
@@ -174,6 +178,18 @@ bool DialogDelegate::DefaultClose() {
   return Accept();
 }
 
+void DialogDelegate::RunCloseCallback(base::OnceClosure callback) {
+  // TODO(ellyjones): It would be better if this instead was:
+  //   DCHECK(!callback_delivered_);
+  // i.e., it was enforced by Views that client code does not cause the
+  // DialogDelegate to be closed from within a closure handler. Unfortunately a
+  // lot of client code does not behave that way right now.
+  if (callback_delivered_)
+    return;
+  callback_delivered_ = true;
+  std::move(callback).Run();
+}
+
 View* DialogDelegate::GetInitiallyFocusedView() {
   // Focus the default button if any.
   const DialogClientView* dcv = GetDialogClientView();

ui/views/window/dialog_delegate.h

@@ -281,6 +281,10 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   // Close() above.
   bool DefaultClose();
 
+  // Runs a close callback, ensuring that at most one close callback is ever
+  // run.
+  void RunCloseCallback(base::OnceClosure callback);
+
   // The margins between the content and the inside of the border.
   // TODO(crbug.com/733040): Most subclasses assume they must set their own
   // margins explicitly, so we set them to 0 here for now to avoid doubled
@@ -307,6 +311,9 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   base::OnceClosure cancel_callback_;
   base::OnceClosure close_callback_;
 
+  // Whether any of the three callbacks just above has been delivered yet.
+  bool callback_delivered_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(DialogDelegate);
 };
 

ui/views/window/dialog_delegate_unittest.cc

@@ -447,4 +447,24 @@ TEST_F(DialogDelegateCloseTest, AnyCallbackInhibitsDefaultClose) {
   EXPECT_FALSE(accepted);
 }
 
+TEST_F(DialogDelegateCloseTest,
+       RecursiveCloseFromAcceptCallbackDoesNotTriggerSecondCallback) {
+  DialogDelegateView dialog;
+
+  bool closed = false;
+  bool accepted = false;
+
+  dialog.set_close_callback(
+      base::BindLambdaForTesting([&]() { closed = true; }));
+  dialog.set_accept_callback(base::BindLambdaForTesting([&]() {
+    accepted = true;
+    dialog.Close();
+  }));
+
+  EXPECT_TRUE(dialog.Accept());
+
+  EXPECT_TRUE(accepted);
+  EXPECT_FALSE(closed);
+}
+
 }  // namespace views