This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

A friendly reminder that M57 Beta launch is coming soon on February 2nd! Your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and get it merged into the release branch (2987) ASAP so it gets enough baking time in Dev (before Beta promotion). Thank you!

jonross, could you please take a look at this security bug? Looks like it might be related to https://codereview.chromium.org/1515203002. Thanks!

I'll take a look. I was looking at the range, expecting a recent change to have addressed this, but clusterfuzz shows it occurring at a later revision, so I'll have to hunt further.

[Bulk edit]

A friendly reminder that M57 Beta launch is coming soon on February 2nd (in a week)! Your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and get it merged into the release branch (2987) ASAP so it gets enough baking time in Dev (before Beta promotion). Thank you!

Review is up: https://codereview.chromium.org/2654093005/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f64dfaabfd764eaca10cc87973f22422a8ee721a

commit f64dfaabfd764eaca10cc87973f22422a8ee721a
Author: jonross <jonross@chromium.org>
Date: Fri Jan 27 00:15:48 2017

Fix MenuRunner Releasing

It is possible for MenuRunner to be released while there is no currently active
MenuController. However the MenuController which it created has not yet been
destroyed.

It is possible for there to be multiple non-destroyed MenuControllers. Due to
this MenuController::GetActiveInstance is not an appropriate way to determine if
the controller you are working with has been destroyed or not.

This change updates MenuController to begin providing a WeakPtr of itself. This
allows its internal code to detect when delegates have deleted it in response
to callbacks. This also allows MenuRunner to track the lifetime of the
controller is created.

This change also updates MenuRunner to make sure that it destroys the
controller it created when it is being released.

TEST=MenuRunnerImplTest.MenuRunnerDestroyedWithNoActiveController
BUG= 683087 

Review-Url: https://codereview.chromium.org/2654093005
Cr-Commit-Position: refs/heads/master@{#446498}

[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/controls/menu/menu_runner_impl.cc
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/controls/menu/menu_runner_impl.h
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/controls/menu/menu_runner_unittest.cc
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/test/menu_test_utils.cc
[modify] https://crrev.com/f64dfaabfd764eaca10cc87973f22422a8ee721a/ui/views/test/menu_test_utils.h

+sky@ and sadrul@ for visibility as I am OOO until Feb 6th.

I have not found the root cause for this issue, as clusterfuzz cannot currently be ran locally. I have a prepared change with extra checks to try to narrow down the cause. (https://codereview.chromium.org/2656823008/) As they should alter the stack trace in clusterfuzz.

Current findings:

MenuRunnerImpl is deleting itself and MenuController is not removing the delegate. A few possible causes
  - MenuController is partially shutdown, ignoring the EXIT_DESTROYED call. Then subsequently the controller becomes reactivated.
  - MenuRunnerImpl no longer thinks that it is running so it does not notify the controller. In this case MenuController told MenuRunnerImpl to shutdown but did not remove the delegate.
  - MenuController is being revived. It is possible that a partially shutdown MenuController is told to run a new menu, before full cleanup.
  - System shutdown leading to nested delete then call to the delegate (see below)

However I am also concerned that this issue can occur in a general system shutdown. The related issue 685462 has slightly different destruction stack trace from the clusterfuzz issue. In its stack trace MenuController releases the ViewsDelegate ref that was keeping Chrome alive. This leads to a nested cancellation of MenuController, triggered by widget destruction, causing the MenuControllerDelegate to be destroyed before the stack collapses.

So we get

MenuRunnerImpl::~MenuRunnerImpl
MenuRunnerImpl::OnMenuClosed
MenuController::ExitAsyncRun
MenuController::Cancel
....
ViewsDelegate::ReleaseRef
MenuController::ExitMenuRun
MenuController::ExitAsyncRun
MenuController::Cancel
...

ExitAsyncRun caches the delegate as ExitMenuRun can lead to MenuController being deleted.
However this means that the cached delegate can be actually deleted by ExitMenuRun during a shutdown.

This leads me to believe that we may still need to use a WeakPtr with MenuControllerDelegate. I have a review with this option if we decide to pursue it: https://codereview.chromium.org/2659903002/

I believe for this to occur in the wild a user would need to have a menu open. Then something else would have to force Chrome to shut. Then when the user clicked outside of the menu, MenuController would allow shutdown to finally occur.

End result would be one use-after-free as Chrome is completely shutting down.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07c6e27a5cea714d7fdfbb297b998a531eb45371

commit 07c6e27a5cea714d7fdfbb297b998a531eb45371
Author: jonross <jonross@chromium.org>
Date: Fri Jan 27 22:31:00 2017

New MenuController Checks

Add additional checks to MenuController and MenuRunnerImpl to help find the root
cause of a use-after-free.

BUG= 683087 

Review-Url: https://codereview.chromium.org/2656823008
Cr-Commit-Position: refs/heads/master@{#446796}

[modify] https://crrev.com/07c6e27a5cea714d7fdfbb297b998a531eb45371/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/07c6e27a5cea714d7fdfbb297b998a531eb45371/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/07c6e27a5cea714d7fdfbb297b998a531eb45371/ui/views/controls/menu/menu_runner_impl.cc

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please merge  your change to M57 branch 2987 ASAP.If merge happens today before 5:00 PM PT, then we can take it for tomorrow's last M57 Dev release. Thank you.

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please merge your change to M57 branch 2987 ASAP (latest before 5:00 PM PT on Wednesday, 02/01/17) so we can pick it up for M57 Beta promotion release this week. Thank you.

CL in #12 was apparently not sufficient and CL in #16 does not need to be merged - it is only adding CHECKs to facilitate more investigation which can be done in Dev channel.
There is a CL with a speculative fix here - https://codereview.chromium.org/2659903002/ but it is not clear if the approach there is correct without knowing via CHECKs in #16.

Removing merge flags for now.

Moving to ReleaseBlock-Stble to give some more time for investigation.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/212e6fd564db2eef90c3855ab96cb75f35cba32d

commit 212e6fd564db2eef90c3855ab96cb75f35cba32d
Author: jonross <jonross@chromium.org>
Date: Tue Feb 07 23:41:21 2017

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}

[modify] https://crrev.com/212e6fd564db2eef90c3855ab96cb75f35cba32d/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/212e6fd564db2eef90c3855ab96cb75f35cba32d/ui/views/controls/menu/menu_runner_impl.h
[modify] https://crrev.com/212e6fd564db2eef90c3855ab96cb75f35cba32d/ui/views/controls/menu/menu_runner_unittest.cc

A friendly reminder that M57 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!

ClusterFuzz has detected this issue as fixed in range 448729:448982.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4946659533651968

Fuzzer: meacer_chromebot_extensions
Job Type: linux_asan_chrome_v8_arm
Platform Id: linux

Crash Type: Heap-use-after-free READ 4
Crash Address: 0xa3317304
Crash State:
  views::MenuController::Cancel
  views::MenuController::RepostEventAndCancel
  views::MenuController::SetSelectionOnPointerDown
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=441524:442831
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=448729:448982

Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv96TFqk7Udrb63lKy9Hp24sJ__z4wfNGwQtNIXYmjA9wnF_8ZNgEb8YsplnXqIGMdbNdscvK961yu90of4Vdj0GYCWMqZYOdF_9W-AVi6P8un5uGiRiW-UBabSI2CFW0C4bfKnyiOcvjmKCCofTFajbY1BA2LwzgbiXLdGURwUo25qL-jOTvwU_SbCXxO5h-ksel69dyl6Q6YCOotdmjTCX1avpw7wnaAscapQDS0KbA7Gkozj_CYYScO814FVUz4rofTbGXKTlKF_xp943hjr0XZu8NyIj_oMWQBlti5rJug3Y8az8x5Onk3RDiEZFD9g57nZs_src1af91I-i-COumnMix3gE44fMHUIi8vD3kByXFVXg?testcase_id=4946659533651968


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Requesting merge for #12 and #24

I'll be reverting the testing in #16

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please merge your change to M57 branch 2987 before 5:00 PM PT, Friday 02/10 (sooner the better please) so we can take it in for next week beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fc4ce03ef87775b0d392d07aa69323e37f82b55

commit 4fc4ce03ef87775b0d392d07aa69323e37f82b55
Author: jonross <jonross@chromium.org>
Date: Thu Feb 09 00:57:07 2017

Revert MenuController Checks
Revert "New MenuController Checks"

This reverts commit 07c6e27a5cea714d7fdfbb297b998a531eb45371.

BUG= 683087 

Review-Url: https://codereview.chromium.org/2687753002
Cr-Commit-Position: refs/heads/master@{#449167}

[modify] https://crrev.com/4fc4ce03ef87775b0d392d07aa69323e37f82b55/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/4fc4ce03ef87775b0d392d07aa69323e37f82b55/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/4fc4ce03ef87775b0d392d07aa69323e37f82b55/ui/views/controls/menu/menu_runner_impl.cc

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28631343e1cc6a3c5d4b32b3117cebe45325c026

commit 28631343e1cc6a3c5d4b32b3117cebe45325c026
Author: jonross <jonross@chromium.org>
Date: Thu Feb 09 16:06:53 2017

Merge MenuRunner Releasing
Fix MenuRunner Releasing

It is possible for MenuRunner to be released while there is no currently active
MenuController. However the MenuController which it created has not yet been
destroyed.

It is possible for there to be multiple non-destroyed MenuControllers. Due to
this MenuController::GetActiveInstance is not an appropriate way to determine if
the controller you are working with has been destroyed or not.

This change updates MenuController to begin providing a WeakPtr of itself. This
allows its internal code to detect when delegates have deleted it in response
to callbacks. This also allows MenuRunner to track the lifetime of the
controller is created.

This change also updates MenuRunner to make sure that it destroys the
controller it created when it is being released.

TEST=MenuRunnerImplTest.MenuRunnerDestroyedWithNoActiveController
BUG= 683087 
TBR=sky@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2654093005
Cr-Commit-Position: refs/heads/master@{#446498}
(cherry picked from commit f64dfaabfd764eaca10cc87973f22422a8ee721a)

Review-Url: https://codereview.chromium.org/2678343012
Cr-Commit-Position: refs/branch-heads/2987@{#406}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/controls/menu/menu_runner_impl.cc
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/controls/menu/menu_runner_impl.h
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/controls/menu/menu_runner_unittest.cc
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/test/menu_test_utils.cc
[modify] https://crrev.com/28631343e1cc6a3c5d4b32b3117cebe45325c026/ui/views/test/menu_test_utils.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b23180daa22d99ae5c6cb2eaf5856e71701f01de

commit b23180daa22d99ae5c6cb2eaf5856e71701f01de
Author: jonross <jonross@chromium.org>
Date: Thu Feb 09 18:15:00 2017

Merge 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 
TBR=sky@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2680863002
Cr-Commit-Position: refs/heads/master@{#448792}
(cherry picked from commit 212e6fd564db2eef90c3855ab96cb75f35cba32d)

Review-Url: https://codereview.chromium.org/2682363002
Cr-Commit-Position: refs/branch-heads/2987@{#410}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/b23180daa22d99ae5c6cb2eaf5856e71701f01de/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/b23180daa22d99ae5c6cb2eaf5856e71701f01de/ui/views/controls/menu/menu_runner_impl.h
[modify] https://crrev.com/b23180daa22d99ae5c6cb2eaf5856e71701f01de/ui/views/controls/menu/menu_runner_unittest.cc

Both required changes are now merged in M-57

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot