Prevent nested Menu Cancelling

Prevent nested Menu Cancelling

When being cancelled MenuController will release ViewsDelegate. This is normally
used to prevent shutdown while menus are alive. The release can lead to views
teardowns attempting to cancel the MenuController and to delete the MenuRunner.

This nested cancelling and deletion of the MenuRunner leads to a use-after-free
once the original cancelling resumes.

This change updates MenuController::Cancel to mark the menu as not showing
earlier in the process. So that nested calls to Cancel are rejected.

TEST=MenuRunnerDestructionTest.MenuRunnerDestroyedDuringReleaseRef
BUG=683087
Review-Url: https://codereview.chromium.org/2680863002
Cr-Commit-Position: refs/heads/master@{#448792}
Committed: https://chromium.googlesource.com/chromium/src/+/212e6fd564db2eef90c3855ab96cb75f35cba32d

[jonross, 2017-02-07 17:03:13]

The CQ bit was checked by jonross@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 17:03:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jonross, 2017-02-07 17:04:56]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-02-07 17:04:56]

Hey,

Previously I added some checks to MenuController to try to find potential causes
of the use-after-free. None of them are being hit.

This leaves the nested shutdown path as a trigger.

In this change I look to preventing this from occurring, by blocking the second
call to Cancel. Rather than converting the MenuControllerDelegate to a WeakPtr

Thoughts?

[commit-bot: I haz the power, 2017-02-07 18:00:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 18:00:59]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-07 19:23:51]

https://codereview.chromium.org/2680863002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2680863002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:2639:
ViewsDelegate::GetInstance()->ReleaseRef();
Is your worry that this line is causing reentrancy that we don't expect? Is the
comment on 2620 wrong? That the delegate has been destroyed too?

[jonross, 2017-02-07 20:07:21]

https://codereview.chromium.org/2680863002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2680863002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:2639:
ViewsDelegate::GetInstance()->ReleaseRef();
On 2017/02/07 19:23:51, sky wrote:
> Is your worry that this line is causing reentrancy that we don't expect? Is
the
> comment on 2620 wrong? That the delegate has been destroyed too?

So I only seem to have a partial stack trace from the fuzzing. 
However the related issue 685462 shows that this line can cause reentrancy of
the MenuController::Cancel.

Specifically:
MenuRunnerImpl::Release()
MenuControllerDelegate::OnMenuClosed()
MenuController::ExitAsyncRun()
MenuController::Cancel(EXIT_DESTROYED)
... a bunch of widget closing calls....
ChromeViewsDelegate::ReleaseRef()
MenuController::ExitAsyncRun()
MenuController::Cancel(EXIT_ALL)
... something that hides the menu, either timer or clicking outside.

The comment on 2620 is not comprehensive. There are three cases:
  1) Releasing ViewsDelegate does nothing, MenuController and
MenuControllerDelegate are both alive after ExitMenuRun
  2) Releasing ViewsDelegate leads to MenuController being destroyed, however
the MenuControllerDelegate is not tied directly to a Widget, and awaits
OnMenuClosed before destroying itself
  3) Releasing ViewsDelegate leads to Widget destruction, which deletes both the
MenuController and the MenuDelegate.

1) is the most most common, no risk.
2) occurs on each shutdown, as the BookmarksBar is built this way, and performs
some related shutdown cleanup here.
3) is what this bug seems to be.

[sky, 2017-02-07 20:51:29]

Ok, LGTM

[jonross, 2017-02-07 21:01:57]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-07 21:04:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 23:41:25]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486501317038940, "parent_rev":
"252bd536aff3249ae046eec0be35b4fc60995756", "commit_rev":
"212e6fd564db2eef90c3855ab96cb75f35cba32d"}

[commit-bot: I haz the power, 2017-02-07 23:42:06]

Description was changed from

==========
Prevent nested Menu Cancelling

When being cancelled MenuController will release ViewsDelegate. This is normally
used to prevent shutdown while menus are alive. The release can lead to views
teardowns attempting to cancel the MenuController and to delete the MenuRunner.

This nested cancelling and deletion of the MenuRunner leads to a use-after-free
once the original cancelling resumes.

This change updates MenuController::Cancel to mark the menu as not showing
earlier in the process. So that nested calls to Cancel are rejected.

TEST=MenuRunnerDestructionTest.MenuRunnerDestroyedDuringReleaseRef
BUG=683087
==========

to

==========
Prevent nested Menu Cancelling

When being cancelled MenuController will release ViewsDelegate. This is normally
used to prevent shutdown while menus are alive. The release can lead to views
teardowns attempting to cancel the MenuController and to delete the MenuRunner.

This nested cancelling and deletion of the MenuRunner leads to a use-after-free
once the original cancelling resumes.

This change updates MenuController::Cancel to mark the menu as not showing
earlier in the process. So that nested calls to Cancel are rejected.

TEST=MenuRunnerDestructionTest.MenuRunnerDestroyedDuringReleaseRef
BUG=683087

Review-Url: https://codereview.chromium.org/2680863002
Cr-Commit-Position: refs/heads/master@{#448792}
Committed:
https://chromium.googlesource.com/chromium/src/+/212e6fd564db2eef90c3855ab96c...
==========

[commit-bot: I haz the power, 2017-02-07 23:42:12]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/212e6fd564db2eef90c3855ab96c...