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

ui/views/widget/widget_interactive_uitest.cc

 // Copyright (c) 2013 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 <stddef.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/macros.h"
@@ skipping 1246 matching lines @@
 
   Widget* widget = delegate.GetWidget();
   ShowSync(widget);
   widget->Show();
   EXPECT_TRUE(delegate.view()->HasFocus());
   EXPECT_EQ(delegate.view(), widget->GetFocusManager()->GetStoredFocusView());
 }
 
 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_;
     got_capture_lost_ = false;
     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);
 };
 
 }  // namespace
 
 class WidgetCaptureTest : public ViewsTestBase {
  public:
   WidgetCaptureTest() {
   }
 
   ~WidgetCaptureTest() override {}
 
   void SetUp() override {
     // On mus these tests run as part of views::ViewsTestSuite which already
     // does this initialization.
     if (!IsMus()) {
       gl::GLSurfaceTestSupport::InitializeOneOff();
       ui::RegisterPathProvider();
       base::FilePath ui_test_pak_path;
       ASSERT_TRUE(PathService::Get(ui::UI_TEST_PAK, &ui_test_pak_path));
       ui::ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path);
     }
     ViewsTestBase::SetUp();
   }
 
   // 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 =
         CreateNativeWidget(params1, use_desktop_native_widget, &widget1);
     params1.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
     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;
     params2.native_widget =
         CreateNativeWidget(params2, use_desktop_native_widget, &widget2);
     widget2.Init(params2);
     widget2.Show();
 
     // Set capture to widget2 and verity it gets it.
     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,
                                    bool create_desktop_native_widget,
                                    Widget* widget) {
     if (create_desktop_native_widget)
       return CreatePlatformDesktopNativeWidgetImpl(params, widget, nullptr);
     return CreatePlatformNativeWidgetImpl(params, widget, kDefault, nullptr);
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(WidgetCaptureTest);
 };
 
 // See description in TestCapture().
 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) {
   // Fails on mus. http://crbug.com/611764
   if (IsMus())
     return;
 
   TestCapture(true);
 }
 #endif
@@ skipping 408 matching lines @@
 
   ui::KeyEvent key_event2(key_event);
   widget->OnKeyEvent(&key_event2);
   EXPECT_FALSE(key_event2.stopped_propagation());
 
   widget->CloseNow();
 }
 
 }  // namespace test
 }  // namespace views