Stop MenuItemView From Directly Caching MenuController

Stop MenuItemView From Directly Caching MenuController

When a MenuController closes its associated Widget is destroyed later on the
MessageLoop. Due to this it is possible for a MenuHostRootView to receive input
while the associated MenuController has been destroyed.

MenuHostRootView accesses the MenuController through MenuItemView which simply
caches it. This change updates MenuItemView to take a WeakPtr to the controller.

TEST=MenuControllerTest.HostReceivesInputBeforeDestruction
BUG=690097
Review-Url: https://codereview.chromium.org/2684993009
Cr-Commit-Position: refs/heads/master@{#449663}
Committed: https://chromium.googlesource.com/chromium/src/+/549b48a07f5ac2def1b8ad0549f3fddaf67b7ff5

[jonross, 2017-02-10 02:49:11]

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

[commit-bot: I haz the power, 2017-02-10 02:49:37]

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-10 02:53:42]

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

[jonross, 2017-02-10 02:53:43]

Hey sky@,

It appears that the MessageLoop can hold input events enqueued for a
MenuHostRootView which is about to destroy itself.

With the MenuController already deleted this ends up causing a crash. Some of
the MenuHostRootView/MenuItemView code is set up to expect nulled controllers.
I'm proposing updating MenuItemView to hold a WeakPtr to the MenuController, so
that it can detect the deletion, and to quietly reject input. 

Subsequently the Widget destruction will clean them up.

Thoughts?

Jon

[commit-bot: I haz the power, 2017-02-10 08:50:28]

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

[commit-bot: I haz the power, 2017-02-10 08:50:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2017-02-10 14:27:31]

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

[commit-bot: I haz the power, 2017-02-10 14:27:59]

Dry run: 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-10 15:22:03]

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

[commit-bot: I haz the power, 2017-02-10 15:22:04]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-10 18:00:29]

LGTM

[jonross, 2017-02-10 18:09:33]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:09:59]

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-10 18:17:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486750173770720, "parent_rev":
"3489b2812bf4e670cc27627ee9a7a1fd1919f399", "commit_rev":
"549b48a07f5ac2def1b8ad0549f3fddaf67b7ff5"}

[commit-bot: I haz the power, 2017-02-10 18:18:03]

Description was changed from

==========
Stop MenuItemView From Directly Caching MenuController

When a MenuController closes its associated Widget is destroyed later on the
MessageLoop. Due to this it is possible for a MenuHostRootView to receive input
while the associated MenuController has been destroyed.

MenuHostRootView accesses the MenuController through MenuItemView which simply
caches it. This change updates MenuItemView to take a WeakPtr to the controller.

TEST=MenuControllerTest.HostReceivesInputBeforeDestruction
BUG=690097
==========

to

==========
Stop MenuItemView From Directly Caching MenuController

When a MenuController closes its associated Widget is destroyed later on the
MessageLoop. Due to this it is possible for a MenuHostRootView to receive input
while the associated MenuController has been destroyed.

MenuHostRootView accesses the MenuController through MenuItemView which simply
caches it. This change updates MenuItemView to take a WeakPtr to the controller.

TEST=MenuControllerTest.HostReceivesInputBeforeDestruction
BUG=690097

Review-Url: https://codereview.chromium.org/2684993009
Cr-Commit-Position: refs/heads/master@{#449663}
Committed:
https://chromium.googlesource.com/chromium/src/+/549b48a07f5ac2def1b8ad0549f3...
==========

[commit-bot: I haz the power, 2017-02-10 18:18:05]

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

[msramek, 2017-02-13 09:25:14]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2690013002/ by msramek@chromium.org.

The reason for reverting is: Significant memory leak caught by ChromeOS
ASan/LSan in the test added by this CL.

For details, please see:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....