Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1876013002:
Fixed potential crash on destroying MenuRunnerImplCocoa. (Closed)
DescriptionFixed potential crash on destroying MenuRunnerImplCocoa.
"Use after free" could happen in MenuRunnerImplCocoa::RunMenu()
if we cancel the menu and destroy it just after canceling
(by calling MenuRunnerImplCocoa::Release()).
In this case after the menu runloop finished we would check
|delete_after_run_| flag that belongs to dead object and then could call
the destructor for it a second time.
BUG=
Committed: https://crrev.com/2c0d33e61f10641ea1984bdc90461c1c439139af
Cr-Commit-Position: refs/heads/master@{#387566}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review fixes. #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after menu runloop finished we were checked |delete_after_run_| flag that belongs to dead object and then could call the destructor for it second time. BUG= ========== to ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after menu runloop finished we were checked |delete_after_run_| flag that belongs to dead object and then could call the destructor for it second time. BUG= ==========
kirr@yandex-team.ru changed reviewers: + andresantoso@chromium.org, sadrul@chromium.org
PTAL. Tests from CL sometimes causes a crash in current code, but works well with patch.
On 2016/04/11 09:53:34, kirr wrote: > PTAL. > > Tests from CL sometimes causes a crash in current code, but works well with > patch. I haven't been in this code in a long time, and I haven't been working in Chromium. I'll let sadrul review this.
andresantoso@chromium.org changed reviewers: - andresantoso@chromium.org
sadrul@chromium.org changed reviewers: + tapted@chromium.org
perhaps +tapted@ can review?
https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right): https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:165: // in MenuRunnerImplCocoa::Release()). this comment is a bit hard to read perhaps // Ensure a menu can be safely released immediately after a call to Cancel() in the same run loop iteration. https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:167: MenuRunner::RunResult result = RunMenu(^{ We got rid of all the Objective C blocks in this file recently - see https://codereview.chromium.org/1829603002/ for the "new way" - you'll need to sync past r385940 and rebase https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:169: runner_->Release(); Is there code in Chrome that already follows this pattern? https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner_impl_cocoa.h (right): https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.h:44: bool in_menu_runloop_; Let's call this running_ -- the comment above even refers to it already :p. (this will more closely match the non-native menu implementation). It should have a separate comment for it too (just copy paste from aura's menu_runner_impl.h - we can keep the order the same too) https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right): https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:104: return [menu_controller_ isMenuOpen]; then I think this should just return running_, to match aura https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:108: if (IsRunning() || in_menu_runloop_) { that way this can stay as just IsRunning() https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:113: [menu_controller_ cancel]; in the current code I think this can be called twice. Cocoa probably copes, but we should't rely on it (or we should at least document that it is fine to call cancel twice). I think we can do if ([menu_controller_ isMenuOpen]) [menu_controller_ cancel] https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:160: [menu_controller_ cancel]; Should this check `if (running_)` before calling `cancel`?
On 2016/04/12 06:22:58, tapted wrote: > ui/views/controls/menu/menu_runner_cocoa_unittest.mm:165: // in > MenuRunnerImplCocoa::Release()). > this comment is a bit hard to read perhaps > // Ensure a menu can be safely released immediately after a call to Cancel() in > the same run loop iteration. Done. > https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... > ui/views/controls/menu/menu_runner_cocoa_unittest.mm:167: MenuRunner::RunResult > result = RunMenu(^{ > We got rid of all the Objective C blocks in this file recently - see > https://codereview.chromium.org/1829603002/ for the "new way" - you'll need to > sync past r385940 and rebase Done. > https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... > ui/views/controls/menu/menu_runner_cocoa_unittest.mm:169: runner_->Release(); > Is there code in Chrome that already follows this pattern? I couldn't find such places in Chrome. There is a case in YB; widget that owns MenuRunner is destroyed in one of menu commands. > https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... > ui/views/controls/menu/menu_runner_impl_cocoa.h:44: bool in_menu_runloop_; > Let's call this running_ -- the comment above even refers to it already :p. > (this will more closely match the non-native menu implementation). > It should have a separate comment for it too (just copy paste from aura's > menu_runner_impl.h - we can keep the order the same too) > then I think this should just return running_, to match aura > that way this can stay as just IsRunning() Done. Thanks. > https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... > ui/views/controls/menu/menu_runner_impl_cocoa.mm:113: [menu_controller_ cancel]; > > I think we can do > if ([menu_controller_ isMenuOpen]) > [menu_controller_ cancel] > > https://codereview.chromium.org/1876013002/diff/1/ui/views/controls/menu/menu... > ui/views/controls/menu/menu_runner_impl_cocoa.mm:160: [menu_controller_ cancel]; > Should this check `if (running_)` before calling `cancel`? It is already checked in https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/cocoa/menu... Seems like this check is enough.
lgtm, but you'll need an OWNER too just some CL description tweaks (sorry to nitpick): - "cancel menu" -> "cancel the menu" - "after menu" -> "after the menu" - "were checked" -> "would check" - "second time" -> "a second time" And it's fine here since the CL is small, but some tips for the future: - it's good practice when rebasing to upload a patchset with "just" the rebase, then upload another that addresses review comments. This makes reviewing the diff easier. - when responding to comments, you can use the links that appear interspersed in the code itself to reply to each one individually rather than responding to the whole text. If there's additional follow-up on a particular review comment, this makes it easier to track. (also thank you for finding and fixing this!)
Description was changed from ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after menu runloop finished we were checked |delete_after_run_| flag that belongs to dead object and then could call the destructor for it second time. BUG= ========== to ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel the menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after the menu runloop finished we would check |delete_after_run_| flag that belongs to dead object and then could call the destructor for it a second time. BUG= ==========
On 2016/04/13 02:45:30, tapted wrote: > lgtm, but you'll need an OWNER too sadrul@, could you take a look? > just some CL description tweaks (sorry to nitpick): > - "cancel menu" -> "cancel the menu" > - "after menu" -> "after the menu" > - "were checked" -> "would check" > - "second time" -> "a second time" > Yep. Sorry for my English. Thank for code review rules :)
ping
ping
lgtm
The CQ bit was checked by kirr@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876013002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel the menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after the menu runloop finished we would check |delete_after_run_| flag that belongs to dead object and then could call the destructor for it a second time. BUG= ========== to ========== Fixed potential crash on destroying MenuRunnerImplCocoa. "Use after free" could happen in MenuRunnerImplCocoa::RunMenu() if we cancel the menu and destroy it just after canceling (by calling MenuRunnerImplCocoa::Release()). In this case after the menu runloop finished we would check |delete_after_run_| flag that belongs to dead object and then could call the destructor for it a second time. BUG= Committed: https://crrev.com/2c0d33e61f10641ea1984bdc90461c1c439139af Cr-Commit-Position: refs/heads/master@{#387566} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2c0d33e61f10641ea1984bdc90461c1c439139af Cr-Commit-Position: refs/heads/master@{#387566} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
