Don't allow a widget to send close notifications more than once.

ui/views/widget/widget_unittest.cc

Index: ui/views/widget/widget_unittest.cc
diff --git a/ui/views/widget/widget_unittest.cc b/ui/views/widget/widget_unittest.cc
index 52845e5ed925fc9141065f1f52e2ac2e7a8ccfc2..ea4f183605b3e571b1103085f9bdcd491714ce35 100644
--- a/ui/views/widget/widget_unittest.cc
+++ b/ui/views/widget/widget_unittest.cc
@@ -738,6 +738,8 @@ class WidgetObserverTest : public WidgetTest, public WidgetObserver {
   void OnWidgetDestroying(Widget* widget) override {
     if (active_ == widget)
       active_ = nullptr;
+    if (widget_activated_ == widget)
+      widget_activated_ = nullptr;
     widget_closed_ = widget;
   }
 
@@ -808,11 +810,10 @@ class WidgetObserverTest : public WidgetTest, public WidgetObserver {
   Widget* widget_to_close_on_hide_;
 };
 
-TEST_F(WidgetObserverTest, DISABLED_ActivationChange) {
+TEST_F(WidgetObserverTest, ActivationChange) {
   WidgetAutoclosePtr toplevel(CreateTopLevelPlatformWidget());
-
-  Widget* toplevel1 = NewWidget();
-  Widget* toplevel2 = NewWidget();
+  WidgetAutoclosePtr toplevel1(NewWidget());
+  WidgetAutoclosePtr toplevel2(NewWidget());
 
   toplevel1->Show();
   toplevel2->Show();
@@ -822,20 +823,79 @@ TEST_F(WidgetObserverTest, DISABLED_ActivationChange) {
   toplevel1->Activate();
 
   RunPendingMessages();
-  EXPECT_EQ(toplevel1, widget_activated());
+  EXPECT_EQ(toplevel1.get(), widget_activated());
 
   toplevel2->Activate();
   RunPendingMessages();
-  EXPECT_EQ(toplevel1, widget_deactivated());
-  EXPECT_EQ(toplevel2, widget_activated());
-  EXPECT_EQ(toplevel2, active());
+  EXPECT_EQ(toplevel1.get(), widget_deactivated());
+  EXPECT_EQ(toplevel2.get(), widget_activated());
+  EXPECT_EQ(toplevel2.get(), active());
 }
 
-TEST_F(WidgetObserverTest, DISABLED_VisibilityChange) {
-  WidgetAutoclosePtr toplevel(CreateTopLevelPlatformWidget());
+namespace {
+
+// This class simulates a focus manager that moves focus to a second widget when
+// the first one is closed. It simulates a situation where a sequence of widget
+// observers might try to call Widget::Close in response to a OnWidgetClosing().
+class WidgetFocusManager : public TestWidgetObserver {

[sky, 2017/04/25 05:04:10]

This name implies it's some how related to views::FocusManager, which it
definitely isn't. Maybe WidgetFocusCloser? Something without FocusManager in the
name.

[Evan Stade, 2017/04/25 22:52:51]

done (WidgetActivationForwarder)

+ public:
+  explicit WidgetFocusManager(Widget* first_widget, Widget* second_widget)

[sky, 2017/04/25 05:04:10]

no explicit.

[Evan Stade, 2017/04/25 22:52:51]

Done.

+      : TestWidgetObserver(first_widget), second_widget_(second_widget) {}
+
+  ~WidgetFocusManager() override {}
+
+ private:
+  // WidgetObserver overrides:
+  void OnWidgetClosing(Widget* widget) override {
+    widget->OnNativeWidgetActivationChanged(false);
+    second_widget_->Activate();
+  }
+  void OnWidgetActivationChanged(Widget* widget, bool active) override {
+    if (!active)
+      widget->Close();
+  }
+
+  Widget* second_widget_;

[sky, 2017/04/25 05:04:10]

optional: widget_to_close_?

[Evan Stade, 2017/04/25 22:52:51]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WidgetFocusManager);
+};
+
+// This class observes a widget and counts the number of times OnWidgetClosing
+// is called.
+class WidgetCloseCounter : public TestWidgetObserver {
+ public:
+  explicit WidgetCloseCounter(Widget* widget) : TestWidgetObserver(widget) {}
+
+  ~WidgetCloseCounter() override {}
+
+  int close_count() const { return close_count_; }
+
+ private:
+  // WidgetObserver overrides:
+  void OnWidgetClosing(Widget* widget) override { close_count_++; }
+
+  int close_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(WidgetCloseCounter);
+};
 
-  Widget* child1 = NewWidget();
-  Widget* child2 = NewWidget();
+}  // namespace
+
+// Test for crbug.com/714334

[sky, 2017/04/25 05:04:10]

I'm ok referencing bugs ids, but I shouldn't need to go to a bug to see what a
test does. The description should stand on its own.

[Evan Stade, 2017/04/25 22:52:51]

Done.

+TEST_F(WidgetObserverTest, CloseReentrancy) {
+  Widget* widget1 = CreateTopLevelPlatformWidget();
+  Widget* widget2 = CreateTopLevelPlatformWidget();
+  WidgetCloseCounter counter(widget1);
+  WidgetFocusManager focus_manager(widget1, widget2);
+  widget1->Close();
+  EXPECT_EQ(1, counter.close_count());
+  widget2->Close();
+}
+
+TEST_F(WidgetObserverTest, VisibilityChange) {
+  WidgetAutoclosePtr toplevel(CreateTopLevelPlatformWidget());
+  WidgetAutoclosePtr child1(NewWidget());
+  WidgetAutoclosePtr child2(NewWidget());
 
   toplevel->Show();
   child1->Show();
@@ -844,16 +904,16 @@ TEST_F(WidgetObserverTest, DISABLED_VisibilityChange) {
   reset();
 
   child1->Hide();
-  EXPECT_EQ(child1, widget_hidden());
+  EXPECT_EQ(child1.get(), widget_hidden());
 
   child2->Hide();
-  EXPECT_EQ(child2, widget_hidden());
+  EXPECT_EQ(child2.get(), widget_hidden());
 
   child1->Show();
-  EXPECT_EQ(child1, widget_shown());
+  EXPECT_EQ(child1.get(), widget_shown());
 
   child2->Show();
-  EXPECT_EQ(child2, widget_shown());
+  EXPECT_EQ(child2.get(), widget_shown());
 }
 
 TEST_F(WidgetObserverTest, DestroyBubble) {