MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose().

ui/views/widget/widget_interactive_uitest.cc

Index: ui/views/widget/widget_interactive_uitest.cc
diff --git a/ui/views/widget/widget_interactive_uitest.cc b/ui/views/widget/widget_interactive_uitest.cc
index 5daea56bee39d591452ba4d6dc85249cc194e2f3..15079b68d4e46e34e2325e35e40c397a135d1a44 100644
--- a/ui/views/widget/widget_interactive_uitest.cc
+++ b/ui/views/widget/widget_interactive_uitest.cc
@@ -1264,11 +1264,11 @@ TEST_F(WidgetTestInteractive, InitialFocus) {
 
 namespace {
 
-// Used to verify OnMouseCaptureLost() has been invoked.
-class CaptureLostTrackingWidget : public Widget {
+// Helper class for CaptureLostTrackingWidget to store whether
+// OnMouseCaptureLost has been invoked for a widget.
+class CaptureLostState {
  public:
-  CaptureLostTrackingWidget() : got_capture_lost_(false) {}
-  ~CaptureLostTrackingWidget() override {}
+  CaptureLostState() : got_capture_lost_(false) {}
 
   bool GetAndClearGotCaptureLost() {
     bool value = got_capture_lost_;
@@ -1276,14 +1276,29 @@ class CaptureLostTrackingWidget : public Widget {
     return value;
   }
 
+  void OnMouseCaptureLost() { got_capture_lost_ = true; }
+
+ private:
+  bool got_capture_lost_;
+
+  DISALLOW_COPY_AND_ASSIGN(CaptureLostState);
+};
+
+// Used to verify OnMouseCaptureLost() has been invoked.
+class CaptureLostTrackingWidget : public Widget {
+ public:
+  CaptureLostTrackingWidget(CaptureLostState* capture_lost_state)

[tapted, 2016/06/24 07:11:58]

nit: explicit

[karandeepb, 2016/06/24 09:27:28]

Done.

+      : capture_lost_state_(capture_lost_state) {}
+
   // Widget:
   void OnMouseCaptureLost() override {
-    got_capture_lost_ = true;
+    capture_lost_state_->OnMouseCaptureLost();
     Widget::OnMouseCaptureLost();
   }
 
  private:
-  bool got_capture_lost_;
+  // Weak. Stores whether OnMouseCaptureLost has been invoked for this widget.
+  CaptureLostState* capture_lost_state_;
 
   DISALLOW_COPY_AND_ASSIGN(CaptureLostTrackingWidget);
 };
@@ -1313,7 +1328,8 @@ class WidgetCaptureTest : public ViewsTestBase {
   // Verifies Widget::SetCapture() results in updating native capture along with
   // invoking the right Widget function.
   void TestCapture(bool use_desktop_native_widget) {
-    CaptureLostTrackingWidget widget1;
+    CaptureLostState capture_state1;
+    CaptureLostTrackingWidget widget1(&capture_state1);
     Widget::InitParams params1 =
         CreateParams(views::Widget::InitParams::TYPE_WINDOW);
     params1.native_widget =
@@ -1322,7 +1338,8 @@ class WidgetCaptureTest : public ViewsTestBase {
     widget1.Init(params1);
     widget1.Show();
 
-    CaptureLostTrackingWidget widget2;
+    CaptureLostState capture_state2;
+    CaptureLostTrackingWidget widget2(&capture_state2);
     Widget::InitParams params2 =
         CreateParams(views::Widget::InitParams::TYPE_WINDOW);
     params2.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
@@ -1335,22 +1352,22 @@ class WidgetCaptureTest : public ViewsTestBase {
     widget2.SetCapture(widget2.GetRootView());
     EXPECT_FALSE(widget1.HasCapture());
     EXPECT_TRUE(widget2.HasCapture());
-    EXPECT_FALSE(widget1.GetAndClearGotCaptureLost());
-    EXPECT_FALSE(widget2.GetAndClearGotCaptureLost());
+    EXPECT_FALSE(capture_state1.GetAndClearGotCaptureLost());
+    EXPECT_FALSE(capture_state2.GetAndClearGotCaptureLost());
 
     // Set capture to widget1 and verify it gets it.
     widget1.SetCapture(widget1.GetRootView());
     EXPECT_TRUE(widget1.HasCapture());
     EXPECT_FALSE(widget2.HasCapture());
-    EXPECT_FALSE(widget1.GetAndClearGotCaptureLost());
-    EXPECT_TRUE(widget2.GetAndClearGotCaptureLost());
+    EXPECT_FALSE(capture_state1.GetAndClearGotCaptureLost());
+    EXPECT_TRUE(capture_state2.GetAndClearGotCaptureLost());
 
     // Release and verify no one has it.
     widget1.ReleaseCapture();
     EXPECT_FALSE(widget1.HasCapture());
     EXPECT_FALSE(widget2.HasCapture());
-    EXPECT_TRUE(widget1.GetAndClearGotCaptureLost());
-    EXPECT_FALSE(widget2.GetAndClearGotCaptureLost());
+    EXPECT_TRUE(capture_state1.GetAndClearGotCaptureLost());
+    EXPECT_FALSE(capture_state2.GetAndClearGotCaptureLost());
   }
 
   NativeWidget* CreateNativeWidget(const Widget::InitParams& params,
@@ -1370,6 +1387,64 @@ TEST_F(WidgetCaptureTest, Capture) {
   TestCapture(false);
 }
 
+// Tests to ensure capture is correctly released from a Widget with capture when
+// it is destroyed. Test for crbug.com/622201.
+TEST_F(WidgetCaptureTest, DestroyWithCapture_CloseNow) {
+  // Fails on mus. http://crbug.com/611764

[tapted, 2016/06/24 07:11:58]

Can you add this to BUG= and give it a brief mention in the CL description -
it's possible you've found the way mus needs to be fixed too :)

[karandeepb, 2016/06/24 09:27:28]

Looking at the Mus bug, It seems to me that the Mus issue is different. They
have both failing tests and crashes. Also this fails because Mus is not calling
Widget::OnMouseCaptureLost from Widget::CloseNow().

+  if (IsMus())
+    return;
+
+  CaptureLostState capture_state;
+  CaptureLostTrackingWidget* widget =
+      new CaptureLostTrackingWidget(&capture_state);
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.native_widget = CreateNativeWidget(params, true, widget);
+  widget->Init(params);
+  widget->Show();
+
+  widget->SetCapture(widget->GetRootView());
+  EXPECT_TRUE(widget->HasCapture());
+  EXPECT_FALSE(capture_state.GetAndClearGotCaptureLost());
+  widget->CloseNow();
+  EXPECT_TRUE(capture_state.GetAndClearGotCaptureLost());
+}
+
+TEST_F(WidgetCaptureTest, DestroyWithCapture_Close) {
+  CaptureLostState capture_state;

[tapted, 2016/06/24 07:11:59]

ooh? so this one passes on mus - interesting (are you sure the crash isn't just
happening "later")?

[karandeepb, 2016/06/24 09:27:28]

The tests don't crash on Mus. They fail because Mus is not calling
Widget::OnMouseCaptureLost from Widget::CloseNow().

[tapted, 2016/06/24 12:14:41]

If there's no crash, it might be better to write the test with that expectation.
i.e. EXPECT_FALSE on the last line of the test if (IsMus()) - that way when it
is fixed on Mus we get a reminder to re-enable the test, rather than potentially
forgetting about it.

[karandeepb, 2016/06/27 02:07:46]

Done.

+  CaptureLostTrackingWidget* widget =
+      new CaptureLostTrackingWidget(&capture_state);
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.native_widget = CreateNativeWidget(params, true, widget);
+  widget->Init(params);
+  widget->Show();
+
+  widget->SetCapture(widget->GetRootView());
+  EXPECT_TRUE(widget->HasCapture());
+  EXPECT_FALSE(capture_state.GetAndClearGotCaptureLost());
+  widget->Close();
+  EXPECT_TRUE(capture_state.GetAndClearGotCaptureLost());
+}
+
+TEST_F(WidgetCaptureTest, DestroyWithCapture_WidgetOwnsNativeWidget) {
+  // Fails on mus. http://crbug.com/611764
+  if (IsMus())
+    return;
+
+  CaptureLostState capture_state;
+  CaptureLostTrackingWidget widget(&capture_state);
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.native_widget = CreateNativeWidget(params, true, &widget);
+  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;

[tapted, 2016/06/24 07:11:59]

it would be nice to avoid some of this boilerplate for setting up the widget..
I'm not sure how much can really be gained, but it could be worth a try. E.g.

 rename CaptureLostState -> CaptureLostHelper


Widget* CaptureLostHelper::ShowNativeOwnsWidget(bool use_desktop_widgets) {
  return CreateWidget(use_desktop_widgets,
Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET;
}

std::unique_ptr<Widget> ShowWidgetOwnsNative() {
  return MakeUnique(CreateWidget(..));
}

private:
Widget* CaptureLostHelper::ShowNativeOwnsWidget(bool use_desktop_widgets,
ownership) {
  ..
}


Maybe it's not worth it - see what you think

[karandeepb, 2016/06/24 09:27:28]

Yeah don't think it worth for just three tests. Could have added this to the
WidgetCaptureTest class, but the tests use different initial params, so there
won't be much gain.

+  widget.Init(params);
+  widget.Show();
+
+  widget.SetCapture(widget.GetRootView());
+  EXPECT_TRUE(widget.HasCapture());
+  EXPECT_FALSE(capture_state.GetAndClearGotCaptureLost());
+  widget.CloseNow();

[tapted, 2016/06/24 07:11:59]

this should test |widget| being deleted / going out of scope, rather than a
CloseNow call

[karandeepb, 2016/06/24 09:27:28]

Done. But then we won't be able to detect if Widget::OnMouseCaptureLost is
called, since its invoked from ~Widget() and hence
CaptureLostTrackingWidget::OnMouseCaptureLost won't be called. Its an
interesting problem though, given a class X to be tested, how do you test ~X()
invokes a function X::A().

[tapted, 2016/06/24 12:14:41]

Ah, yeah that's a good point :/. But I think there is a way :).
Widget::CreateRootView() is virtual, so you could inherit from
internal::RootView and override OnMouseCaptureLost there, then provide that in
CaptureLostTrackingWidget. That's probably overkill though -
WIDGET_OWNS_NATIVE_WIDGET is mostly used in tests, and not much interesting
happens after destruction, so the important thing is that there's no crash,
which this covers.

+  EXPECT_TRUE(capture_state.GetAndClearGotCaptureLost());
+}
+
 #if !defined(OS_CHROMEOS)
 // See description in TestCapture(). Creates DesktopNativeWidget.
 TEST_F(WidgetCaptureTest, CaptureDesktopNativeWidget) {