Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090003002:
MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose().
Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a
OnMouseCaptureLost callback from its CocoaMouseCapture member instance.
BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has
already been destroyed, leading to a use after crash.
This CL resolves the crash by explicitly releasing mouse capture in
BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on
MacViews for the current master, to test the various codepaths possible when a
window is closed.
BUG=622201
Committed: https://crrev.com/dc6e0034dc928d3c10ee923284d0f10781975280
Cr-Commit-Position: refs/heads/master@{#402385}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review comments. #Patch Set 3 : Address review comments. #
Total comments: 2
Patch Set 4 : Fix trybots. Don't run tests on Mus. #
Total comments: 13
Patch Set 5 : Address review comments. #Patch Set 6 : Address review. #Patch Set 7 : Rebase. #
Dependent Patchsets: Messages
Total messages: 30 (16 generated)
Description was changed from ========== Use after crash. ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. BUG=622201 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. Also added a test which crashed on MacViews for the current master. BUG=622201 ==========
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. Also added a test which crashed on MacViews for the current master. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. Also added a test which crashes on MacViews for the current master. BUG=622201 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. As for your comment on the bug report about tests that we should have failed already, fixing the three mouse capture tests that we fail currently, helped me find this issue.
https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:369: // If we have capture, we'll get a OnMouseCaptureLost callback, which can nit: it's generally good to avoid "we". (there's even go/avoidwe). Here "we" actually refers to specific things so it's better to say what those things are. E.g. you can say // Ensure BridgedNativeWidget does not have capture, otherwise OnMouseCaptureLost() will reference a deleted Widget when called via ~CocoaMouseCapture() upon the destruction of |mouse_capture_|. https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:372: ReleaseCapture(); DesktopNativeWidgetAura does it in OnHostClosed, which is closer to our OnWindowWillClose - is there a reason to be different? (maybe this is better, but there are lots of codepaths) In particular, we need to see if Aura invokes Widget::OnMouseCaptureLost during shutdown-with-capture, and try to be consistent with that. There's a CaptureLostTrackingWidget in widget_interactive_uitest.cc which can help with this. https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1777: // Verify capture is correctly set on a widget. Test for crbug.com/622201. This doesn't describe the test well. It should be something like "Test to ensure capture is correctly released from a Widget with capture when it is destroyed." https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1779: Widget* widget = CreateTopLevelNativeWidget(); We should use CreateNativeDesktopWidget() for better coverage. But it will probably mean that the test needs to go into widget_interactive_uitest.cc https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1783: widget->CloseNow(); We should test asynchronous Close() as well, since the codepath is pretty different. We should probably also test InitParams::WIDGET_OWNS_NATIVE_WIDGET :/
Patchset #2 (id:40001) has been deleted
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget destructor. Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in the BridgedNativeWidget destructor. Also added a test which crashes on MacViews for the current master. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ==========
Patchset #2 (id:60001) has been deleted
PTAL Trent. Thanks. https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:369: // If we have capture, we'll get a OnMouseCaptureLost callback, which can On 2016/06/23 02:26:28, tapted wrote: > nit: it's generally good to avoid "we". (there's even go/avoidwe). Here "we" > actually refers to specific things so it's better to say what those things are. > E.g. you can say > > // Ensure BridgedNativeWidget does not have capture, otherwise > OnMouseCaptureLost() will reference a deleted Widget when called via > ~CocoaMouseCapture() upon the destruction of |mouse_capture_|. Done. https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:372: ReleaseCapture(); On 2016/06/23 02:26:28, tapted wrote: > DesktopNativeWidgetAura does it in OnHostClosed, which is closer to our > OnWindowWillClose - is there a reason to be different? > > (maybe this is better, but there are lots of codepaths) > > In particular, we need to see if Aura invokes Widget::OnMouseCaptureLost during > shutdown-with-capture, and try to be consistent with that. There's a > CaptureLostTrackingWidget in widget_interactive_uitest.cc which can help with > this. Yes Widget::OnMouseCaptureLost is invoked for aura during shutdown-with-capture. However I am not sure if testing that is relevant to this issue. Various code-paths are possible depending on Close/CloseNow or whether it is a DesktopNativeWidgetAura or NativeWidgetAura. For example, for DesktopNativeWidgetAura, for a normal CloseNow call, the flow is DesktopNativeWidgetAura::CloseNow() -> DesktopWindowTreeHostX11::CloseNow() -> DesktopWindowTreeHostX11::ReleaseCapture(). Have moved this to OnWindowWillClose. Both should be fine, we just need to ensure that we release capture before the native_widget_mac_ is destroyed. https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1777: // Verify capture is correctly set on a widget. Test for crbug.com/622201. On 2016/06/23 02:26:28, tapted wrote: > This doesn't describe the test well. It should be something like "Test to ensure > capture is correctly released from a Widget with capture when it is destroyed." Done. https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1779: Widget* widget = CreateTopLevelNativeWidget(); On 2016/06/23 02:26:28, tapted wrote: > We should use CreateNativeDesktopWidget() for better coverage. But it will > probably mean that the test needs to go into widget_interactive_uitest.cc Why is moving to widget_interactive_uitest.cc needed? Also between CreateNativeDesktopWidget/CreateTopLevelNativeWidget, isn't it just that one uses DesktopNativeWidgetAura and the other NativeWidgetAura? https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1783: widget->CloseNow(); On 2016/06/23 02:26:28, tapted wrote: > We should test asynchronous Close() as well, since the codepath is pretty > different. > > We should probably also test InitParams::WIDGET_OWNS_NATIVE_WIDGET :/ Done.
Please hold off the review. Seem to have found something.
Patchset #3 (id:100001) has been deleted
PTAL Trent. Thanks. https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:372: ReleaseCapture(); On 2016/06/23 07:58:32, karandeepb wrote: > On 2016/06/23 02:26:28, tapted wrote: > > DesktopNativeWidgetAura does it in OnHostClosed, which is closer to our > > OnWindowWillClose - is there a reason to be different? > > > > (maybe this is better, but there are lots of codepaths) > > > > In particular, we need to see if Aura invokes Widget::OnMouseCaptureLost > during > > shutdown-with-capture, and try to be consistent with that. There's a > > CaptureLostTrackingWidget in widget_interactive_uitest.cc which can help with > > this. > > Yes Widget::OnMouseCaptureLost is invoked for aura during shutdown-with-capture. > However I am not sure if testing that is relevant to this issue. > Various code-paths are possible depending on Close/CloseNow or whether it is a > DesktopNativeWidgetAura or NativeWidgetAura. For example, for > DesktopNativeWidgetAura, for a normal CloseNow call, the flow is > DesktopNativeWidgetAura::CloseNow() -> DesktopWindowTreeHostX11::CloseNow() -> > DesktopWindowTreeHostX11::ReleaseCapture(). > > Have moved this to OnWindowWillClose. Both should be fine, we just need to > ensure that we release capture before the native_widget_mac_ is destroyed. Done. https://codereview.chromium.org/2090003002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2090003002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:366: [window_ setDelegate:nil]; Is setting the delegate to nil so early really necessary, instead of in OnWindowWillClose()? https://codereview.chromium.org/2090003002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:378: [window_ close]; This does not do a [window_ orderOut:nil] like in NativeWidgetMac::Close(). Is this intentional? Had we did a orderOut with a non-nil delegate, this bug would not have been caused, since we release capture when visibility state is set to hidden.
lgtm if there are no surprises with the WIDGET_OWNS change https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2090003002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:1779: Widget* widget = CreateTopLevelNativeWidget(); On 2016/06/23 07:58:32, karandeepb wrote: > On 2016/06/23 02:26:28, tapted wrote: > > We should use CreateNativeDesktopWidget() for better coverage. But it will > > probably mean that the test needs to go into widget_interactive_uitest.cc > > Why is moving to widget_interactive_uitest.cc needed? Capture has complex interactions with the OS sometimes. E.g. I think that another window activating on Windows can cause capture to be lost unexpectedly. > Also between > CreateNativeDesktopWidget/CreateTopLevelNativeWidget, isn't it just that one > uses DesktopNativeWidgetAura and the other NativeWidgetAura? Yup. The problem is that testing NativeWidgetAura on Windows/Linux tends not to be very exciting - we miss out on a lot of coverage, and it's somewhat redundant with the tests also running on ChromeOS. Desktop Widgets mean not only DesktopNativeWidgetAura gets coverage, but also DesktopWindowTreeHostX11 DesktopWindowTreeHostWin HWNDMessageHandler and a bunch of other things I actually want to convert more of the widget_unittests to be parameterized so they run both with/without desktop widgets. I think I made a bug for it.. somewhere. https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1290: CaptureLostTrackingWidget(CaptureLostState* capture_lost_state) nit: explicit https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1393: // Fails on mus. http://crbug.com/611764 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 :) https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1413: CaptureLostState capture_state; ooh? so this one passes on mus - interesting (are you sure the crash isn't just happening "later")? https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1437: params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; 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 https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1444: widget.CloseNow(); this should test |widget| being deleted / going out of scope, rather than a CloseNow call
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201, 611764 ==========
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201, 611764 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ==========
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL Trent in case you have any more comments. +sky for ui/views/widget/widget_interactive_uitest.cc review. https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1290: CaptureLostTrackingWidget(CaptureLostState* capture_lost_state) On 2016/06/24 07:11:58, tapted wrote: > nit: explicit Done. https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1393: // Fails on mus. http://crbug.com/611764 On 2016/06/24 07:11:58, tapted wrote: > 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 :) 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(). https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1413: CaptureLostState capture_state; On 2016/06/24 07:11:59, tapted wrote: > ooh? so this one passes on mus - interesting (are you sure the crash isn't just > happening "later")? The tests don't crash on Mus. They fail because Mus is not calling Widget::OnMouseCaptureLost from Widget::CloseNow(). https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1437: params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; On 2016/06/24 07:11:59, tapted wrote: > 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 > 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. https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1444: widget.CloseNow(); On 2016/06/24 07:11:59, tapted wrote: > this should test |widget| being deleted / going out of scope, rather than a > CloseNow call 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().
https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1413: CaptureLostState capture_state; On 2016/06/24 09:27:28, karandeepb wrote: > On 2016/06/24 07:11:59, tapted wrote: > > ooh? so this one passes on mus - interesting (are you sure the crash isn't > just > > happening "later")? > > The tests don't crash on Mus. They fail because Mus is not calling > Widget::OnMouseCaptureLost from Widget::CloseNow(). 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. https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1444: widget.CloseNow(); On 2016/06/24 09:27:28, karandeepb wrote: > On 2016/06/24 07:11:59, tapted wrote: > > this should test |widget| being deleted / going out of scope, rather than a > > CloseNow call > > 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(). 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.
https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2090003002/diff/140001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1413: CaptureLostState capture_state; On 2016/06/24 12:14:41, tapted wrote: > On 2016/06/24 09:27:28, karandeepb wrote: > > On 2016/06/24 07:11:59, tapted wrote: > > > ooh? so this one passes on mus - interesting (are you sure the crash isn't > > just > > > happening "later")? > > > > The tests don't crash on Mus. They fail because Mus is not calling > > Widget::OnMouseCaptureLost from Widget::CloseNow(). > > 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. Done.
ping sky@ for ui/views/widget/widget_interactive_uitest.cc review.
LGTM
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2090003002/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 ========== to ========== MacViews: Explicitly release mouse capture in BridgedNativeWidget::OnWindowWillClose(). Currently, if a BridgedNativeWidget with mouse capture is destroyed, it gets a OnMouseCaptureLost callback from its CocoaMouseCapture member instance. BridgedNativeWidget::OnMouseCaptureLost accesses native_widget_mac_ which has already been destroyed, leading to a use after crash. This CL resolves the crash by explicitly releasing mouse capture in BridgedNativeWidget::OnWindowWillClose(). Also added tests which crash on MacViews for the current master, to test the various codepaths possible when a window is closed. BUG=622201 Committed: https://crrev.com/dc6e0034dc928d3c10ee923284d0f10781975280 Cr-Commit-Position: refs/heads/master@{#402385} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dc6e0034dc928d3c10ee923284d0f10781975280 Cr-Commit-Position: refs/heads/master@{#402385} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
