Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
214083004:
Prevents crash when a menu gets closed during drag (Closed)
|
Created:
6 years, 9 months ago by varkha Modified:
6 years, 8 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPrevents crash when a menu gets closed during drag. This only gets exposed with patch https://codereview.chromium.org/196213004/ applied. DesktopNativeWidgetAura::ViewRemoved should do the same as NativeWidgetAura::ViewRemoved and clean up the drag state by calling ResetTargetViewIfEquals for the submenu view.
The use case that exposes this is when a bookmark bar model gets reloaded (and menu drop down is closed) during a drag of a bookmark over the menu.
BUG=349154
TEST=Open a new browser on a new tab page and immediately drag a bookmark over a bookmark bar drop down menu. After a few seconds the menu may close when bookmark menu model gets synced but the browser should not crash.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260751
Patch Set 1 #Patch Set 2 : Prevents crash when a menu gets closed during drag (typo) #
Total comments: 4
Patch Set 3 : Prevents crash when a menu gets closed during drag (removed unnecessary part of the change) #
Total comments: 1
Messages
Total messages: 19 (0 generated)
sadrul@, sky@, can you please take a look? I am trying to land this before the M35 branch, I will file a separate bug about test coverage for bookmark dragging and see what tests can be added in short term. Thanks! https://codereview.chromium.org/214083004/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/214083004/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:595: drop_helper_->ResetTargetViewIfEquals(view); This is same as the code in NativeWidgetAura::ViewRemoved and is here for the same reason. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); I think this is a deficiency in how focus is cleaning things up and you shouldn't work around it like this.
https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); On 2014/03/27 21:40:43, sky wrote: > I think this is a deficiency in how focus is cleaning things up and you > shouldn't work around it like this. In this case the menu does not have focus (the menu is dropped down by the drag over it, not by a click) and the focus can be anywhere else - even in a different instance. The crash happens in DropHelper::NotifyDragExit() because the DropHelper::target_view_ (of the SubmenuView class) is still not NULL but has been destroyed when the menu was closed and failed to notify the Widget. Should we call NativeWidgetPrivate::ViewRemoved in some other code path?
https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); On 2014/03/27 22:05:10, varkha wrote: > On 2014/03/27 21:40:43, sky wrote: > > I think this is a deficiency in how focus is cleaning things up and you > > shouldn't work around it like this. > > In this case the menu does not have focus (the menu is dropped down by the drag > over it, not by a click) and the focus can be anywhere else - even in a > different instance. The crash happens in DropHelper::NotifyDragExit() because > the DropHelper::target_view_ (of the SubmenuView class) is still not NULL but > has been destroyed when the menu was closed and failed to notify the Widget. > Should we call NativeWidgetPrivate::ViewRemoved in some other code path? Isn't this happening because SubmenuView is client owned so that when the RootView is deleted no notification is sent for the SubmenuView?
On 2014/03/27 23:31:28, sky wrote: > https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... > File ui/views/controls/menu/menu_host.cc (right): > > https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... > ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); > On 2014/03/27 22:05:10, varkha wrote: > > On 2014/03/27 21:40:43, sky wrote: > > > I think this is a deficiency in how focus is cleaning things up and you > > > shouldn't work around it like this. > > > > In this case the menu does not have focus (the menu is dropped down by the > drag > > over it, not by a click) and the focus can be anywhere else - even in a > > different instance. The crash happens in DropHelper::NotifyDragExit() because > > the DropHelper::target_view_ (of the SubmenuView class) is still not NULL but > > has been destroyed when the menu was closed and failed to notify the Widget. > > Should we call NativeWidgetPrivate::ViewRemoved in some other code path? > > Isn't this happening because SubmenuView is client owned so that when the > RootView is deleted no notification is sent for the SubmenuView? Yes, but I thought this was intentional - we didn't want SubmenuView to be a child view. Was it intentional?
On Thu, Mar 27, 2014 at 4:59 PM, <varkha@chromium.org> wrote: > On 2014/03/27 23:31:28, sky wrote: > > https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... >> >> File ui/views/controls/menu/menu_host.cc (right): > > > > https://codereview.chromium.org/214083004/diff/20001/ui/views/controls/menu/m... >> >> ui/views/controls/menu/menu_host.cc:98: ViewHierarchyChanged(details); >> On 2014/03/27 22:05:10, varkha wrote: >> > On 2014/03/27 21:40:43, sky wrote: >> > > I think this is a deficiency in how focus is cleaning things up and >> > > you >> > > shouldn't work around it like this. >> > >> > In this case the menu does not have focus (the menu is dropped down by >> > the >> drag >> > over it, not by a click) and the focus can be anywhere else - even in a >> > different instance. The crash happens in DropHelper::NotifyDragExit() > > because >> >> > the DropHelper::target_view_ (of the SubmenuView class) is still not >> > NULL > > but >> >> > has been destroyed when the menu was closed and failed to notify the >> > Widget. >> > Should we call NativeWidgetPrivate::ViewRemoved in some other code path? > > >> Isn't this happening because SubmenuView is client owned so that when the >> RootView is deleted no notification is sent for the SubmenuView? > > > Yes, but I thought this was intentional - we didn't want SubmenuView to be a > child view. SubmenuView is definitely a child a view (well a descendant, it's a child of MenuScrollViewContainer::MenuScrollView). I thought there was a problem in ViewHierarchyChanged not being sent because SubmenuView (and a few other classes) are client owned, but I'm not sure that is the case now. So, the question becomes why isn't Widget getting ViewHierarchyChanged here? Also, the place you're adding the code is in Close(), which means the Widget is about to be destroyed anyway. Are you sure this is needed? If so, you're going to have to debug why ViewHiearchyChanged isn't being sent. -Scott > > https://codereview.chromium.org/214083004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sky@, the part of the change that was confusing proved to be unnecessary and wrong. The views hierarchy is actually properly traversed and ViewRemoved is called on the child views. The only problem was that DesktopNativeWidgetAura::ViewRemoved was not implemented and the drag context was not properly cleaned. This seems like a simple omission now when code in NativeWidgetAura was not ported to DesktopNativeWidgetAura. PTAL. https://codereview.chromium.org/214083004/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/214083004/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:594: drop_helper_->ResetTargetViewIfEquals(view); This is same as the code in NativeWidgetAura::ViewRemoved and is here for the same reason. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
Makes sense, LGTM
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/214083004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/214083004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/214083004/60001
Message was sent while issue was closed.
Change committed as 260751 |
