Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1515203002:
Update closing of nested asynchronous menus (Closed)
DescriptionUpdate closing of nested asynchronous menus
Two updates to the closing of menus running asynchronously.
Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called.
Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran.
TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested
BUG=557130
Committed: https://crrev.com/ac5eac3ea6fa6585d4b2900e39bf5f491ac36570
Cr-Commit-Position: refs/heads/master@{#369536}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Fix Memory Crash #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rebase #
Depends on Patchset: Messages
Total messages: 19 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.DoubleAsynchronousNested BUG=557130 ========== to ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested BUG=557130 ==========
jonross@chromium.org changed reviewers: + mohsen@chromium.org, sky@chromium.org
Hi, Could you provide an owner's review for this change? I have updated MenuController exiting for nested asynchronous menus. To properly handle EXIT_ALL. I have also updated the closing of drag/drop operations when the underlying menu is asynchronous. To ensure proper destruction order. Thanks, Jon
https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:814: // ExitAsyncRun can delete us, however CloseAllNestedMenus enqueues work which I'm nervous of posting the task. What work is causing problems? Can we move the deletion later on so as to avoid the delayed delete?
https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:814: // ExitAsyncRun can delete us, however CloseAllNestedMenus enqueues work which On 2015/12/11 18:37:46, sky wrote: > I'm nervous of posting the task. What work is causing problems? Can we move the > deletion later on so as to avoid the delayed delete? The shutdown enqueues the closing the current widget. Which ends up running within the message loop that ash's drag/drop controller started. The subsequent deletion of the MenuController leads to a corrupted buffer that is attempted to be released a second time. During a subsequent frame render. When MenuController was nesting message loops itself the deletion would occur after the drag/drop loop was gone, as well as after its own loop is gone. (SetExitType terminates the loop) Without MenuController starting up a loop is receives no further notification from drag/drop after OnPerformDrop to trigger the deletion from. That's why I chose to post a task here. Any suggestions on a better way to delay the delete?
https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:814: // ExitAsyncRun can delete us, however CloseAllNestedMenus enqueues work which On 2016/01/06 21:18:31, jonross wrote: > On 2015/12/11 18:37:46, sky wrote: > > I'm nervous of posting the task. What work is causing problems? Can we move > the > > deletion later on so as to avoid the delayed delete? > > The shutdown enqueues the closing the current widget. Which ends up running > within the message loop that ash's drag/drop controller started. > > The subsequent deletion of the MenuController leads to a corrupted buffer that > is attempted to be released a second time. During a subsequent frame render. What do you mean by 'corrupted buffer that is released a second time'? > When MenuController was nesting message loops itself the deletion would occur > after the drag/drop loop was gone, as well as after its own loop is gone. > (SetExitType terminates the loop) > > Without MenuController starting up a loop is receives no further notification > from drag/drop after OnPerformDrop to trigger the deletion from. Shouldn't it close itself before calling to the delegate (as IsBlockingRun()) does above? > > That's why I chose to post a task here. Any suggestions on a better way to delay > the delete?
https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:814: // ExitAsyncRun can delete us, however CloseAllNestedMenus enqueues work which On 2016/01/06 22:20:56, sky wrote: > On 2016/01/06 21:18:31, jonross wrote: > > On 2015/12/11 18:37:46, sky wrote: > > > I'm nervous of posting the task. What work is causing problems? Can we move > > the > > > deletion later on so as to avoid the delayed delete? > > > > The shutdown enqueues the closing the current widget. Which ends up running > > within the message loop that ash's drag/drop controller started. > > > > The subsequent deletion of the MenuController leads to a corrupted buffer that > > is attempted to be released a second time. During a subsequent frame render. > > What do you mean by 'corrupted buffer that is released a second time'? > > > When MenuController was nesting message loops itself the deletion would occur > > after the drag/drop loop was gone, as well as after its own loop is gone. > > (SetExitType terminates the loop) > > > > Without MenuController starting up a loop is receives no further notification > > from drag/drop after OnPerformDrop to trigger the deletion from. > > Shouldn't it close itself before calling to the delegate (as IsBlockingRun()) > does above? > > > > > That's why I chose to post a task here. Any suggestions on a better way to > delay > > the delete? > I'm seeing a few memory errors: - Found a corrupted memory buffer in MallocBlock - Memory was written to after being freed Which is being triggered in cc/layers/picture_layer.cc PictureLayer::PushPropertiesTo This occurs after DragDropController::StartDragAndDrop has returned from its message loop. During a subsequent paint. As for closing itself before calling the delegate, the IsBlockingRun is not hit in the case of the crash as that is tied to opening the menu for drag/drop. In this case: - Wrench menu is created as async - The Bookmarks submenu is opened - Perform a drag/drop in the bookmarks submenu. However you are correct, MenuController should close itself before signalling the delegate. ExitAsyncMenu does to this.
Did you try running asan to understand why the crash is happening? On Fri, Jan 8, 2016 at 12:52 PM, <jonross@chromium.org> wrote: > > https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/1515203002/diff/20001/ui/views/controls/menu/... > ui/views/controls/menu/menu_controller.cc:814: // ExitAsyncRun can > delete us, however CloseAllNestedMenus enqueues work which > On 2016/01/06 22:20:56, sky wrote: >> >> On 2016/01/06 21:18:31, jonross wrote: >> > On 2015/12/11 18:37:46, sky wrote: >> > > I'm nervous of posting the task. What work is causing problems? > > Can we move >> >> > the >> > > deletion later on so as to avoid the delayed delete? >> > >> > The shutdown enqueues the closing the current widget. Which ends up > > running >> >> > within the message loop that ash's drag/drop controller started. >> > >> > The subsequent deletion of the MenuController leads to a corrupted > > buffer that >> >> > is attempted to be released a second time. During a subsequent frame > > render. > >> What do you mean by 'corrupted buffer that is released a second time'? > > >> > When MenuController was nesting message loops itself the deletion > > would occur >> >> > after the drag/drop loop was gone, as well as after its own loop is > > gone. >> >> > (SetExitType terminates the loop) >> > >> > Without MenuController starting up a loop is receives no further > > notification >> >> > from drag/drop after OnPerformDrop to trigger the deletion from. > > >> Shouldn't it close itself before calling to the delegate (as > > IsBlockingRun()) >> >> does above? > > >> > >> > That's why I chose to post a task here. Any suggestions on a better > > way to >> >> delay >> > the delete? > > > > I'm seeing a few memory errors: > - Found a corrupted memory buffer in MallocBlock > - Memory was written to after being freed > Which is being triggered in cc/layers/picture_layer.cc > PictureLayer::PushPropertiesTo > > This occurs after DragDropController::StartDragAndDrop has returned from > its message loop. During a subsequent paint. > > As for closing itself before calling the delegate, the IsBlockingRun is > not hit in the case of the crash as that is tied to opening the menu for > drag/drop. In this case: > - Wrench menu is created as async > - The Bookmarks submenu is opened > - Perform a drag/drop in the bookmarks submenu. > > However you are correct, MenuController should close itself before > signalling the delegate. ExitAsyncMenu does to this. > > https://codereview.chromium.org/1515203002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No I hadn't ran under ASAN, I forgot to. Apparently the failure being reported from a painting stack trace completely mislead me. The failure was a write-after-free inside of MenuController. I've found and fixed that. I've also removed the deferred calls to ExitAsyncRun. They are now all direct calls.
LGTM https://codereview.chromium.org/1515203002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1072: if (GetActiveInstance()) Please add a comment as to why this check is necessary.
https://codereview.chromium.org/1515203002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1515203002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1072: if (GetActiveInstance()) On 2016/01/11 20:56:04, sky wrote: > Please add a comment as to why this check is necessary. Done.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1515203002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1515203002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1515203002/80001
Message was sent while issue was closed.
Description was changed from ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested BUG=557130 ========== to ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested BUG=557130 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested BUG=557130 ========== to ========== Update closing of nested asynchronous menus Two updates to the closing of menus running asynchronously. Update the handling of EXIT_ALL when async menus are nested within others, so that MenuControllerDelegate::OnMenuClosed is properly called. Update closing of an async menu that is performing drag/drop to properly notify MenuControllerDelegate::OnMenuClosed, once hide animations have ran. TEST=MenuControllerTest.AsynchronousPerformDrop, MenuControllerTest.AsynchronousDragComplete, MenuControllerTest.DoubleAsynchronousNested BUG=557130 Committed: https://crrev.com/ac5eac3ea6fa6585d4b2900e39bf5f491ac36570 Cr-Commit-Position: refs/heads/master@{#369536} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ac5eac3ea6fa6585d4b2900e39bf5f491ac36570 Cr-Commit-Position: refs/heads/master@{#369536} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
