Fix 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
Review-Url: https://codereview.chromium.org/2654093005
Cr-Commit-Position: refs/heads/master@{#446498}
Committed: https://chromium.googlesource.com/chromium/src/+/f64dfaabfd764eaca10cc87973f22422a8ee721a

[jonross, 2017-01-26 17:53:42]

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

[commit-bot: I haz the power, 2017-01-26 17:54:05]

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

[jonross, 2017-01-26 17:54:30]

Description was changed from

==========
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=
BUG=683087
==========

to

==========
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
==========

[jonross, 2017-01-26 19:06:06]

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

[commit-bot: I haz the power, 2017-01-26 19:06:52]

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-01-26 20:03:38]

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

[commit-bot: I haz the power, 2017-01-26 20:03:40]

Dry run: This issue passed the CQ dry run.

[jonross, 2017-01-26 20:27:49]

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

[jonross, 2017-01-26 20:27:50]

Hey sky@,

Could you review this change to MenuController/MenuRunner. It switches to using
WeakPtr for the controller do help deal with weird destruction order.

Thanks,
Jon

[jonross, 2017-01-26 20:27:51]

Hey sky@,

Could you review this change to MenuController/MenuRunner. It switches to using
WeakPtr for the controller do help deal with weird destruction order.

Thanks,
Jon

[jonross, 2017-01-26 20:27:52]

Hey sky@,

Could you review this change to MenuController/MenuRunner. It switches to using
WeakPtr for the controller do help deal with weird destruction order.

Thanks,
Jon

[sky, 2017-01-26 22:08:27]

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

https://codereview.chromium.org/2654093005/diff/20001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1030: base::WeakPtr<MenuController>
active_controller =
How come you grab a WeakPtr for the active instance and not this?

It seems this should be:

if (GetActiveController() == this) {
  base::WeakPtr<MenuController> this_ref = AsWeakPtr();
  CloseAllNestedMenus();
  Cancel(EXIT_ALL);
  if (!this_ref)
    return;
  ExitAsyncRun();
}

That clarifies this can be deleted.

https://codereview.chromium.org/2654093005/diff/20001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1069: base::WeakPtr<MenuController>
controller = AsWeakPtr();
The name controller doesn't convey it's the same as this. Maybe this_ref?

[jonross, 2017-01-26 22:26:58]

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

https://codereview.chromium.org/2654093005/diff/20001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1030: base::WeakPtr<MenuController>
active_controller =
On 2017/01/26 22:08:27, sky wrote:
> How come you grab a WeakPtr for the active instance and not this?
> 
> It seems this should be:
> 
> if (GetActiveController() == this) {
>   base::WeakPtr<MenuController> this_ref = AsWeakPtr();
>   CloseAllNestedMenus();
>   Cancel(EXIT_ALL);
>   if (!this_ref)
>     return;
>   ExitAsyncRun();
> }
> 
> That clarifies this can be deleted.

Yeah that is cleaner. Done.

https://codereview.chromium.org/2654093005/diff/20001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1069: base::WeakPtr<MenuController>
controller = AsWeakPtr();
On 2017/01/26 22:08:27, sky wrote:
> The name controller doesn't convey it's the same as this. Maybe this_ref?

Yeah I was iffy on the naming. I like "this_ref" going with that.

[sky, 2017-01-26 23:03:27]

LGTM

[jonross, 2017-01-26 23:15:19]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-01-26 23:15:54]

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-01-27 00:15:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485472519208000,
"parent_rev": "7c6a615b2b23452b33af668caab3945fe44b52a4", "commit_rev":
"f64dfaabfd764eaca10cc87973f22422a8ee721a"}

[commit-bot: I haz the power, 2017-01-27 00:16:29]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/f64dfaabfd764eaca10cc87973f2...
==========

[commit-bot: I haz the power, 2017-01-27 00:16:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f64dfaabfd764eaca10cc87973f2...