[Cocoa] Crash fix, caused by recent tab indicator change.

[Cocoa] Crash fix, caused by recent tab indicator change.

I misunderstood how ObjectiveC handled pointers declared with __weak.
This change resolves the issue by nil'ing things out in
[TabController dealloc], the standard solution.

Introduced by: https://codereview.chromium.org/688523002/

BUG=429605
Committed: https://crrev.com/b85fc6de11e5e162864d58a32da6a02a9a24ea3a
Cr-Commit-Position: refs/heads/master@{#302536}

[miu, 2014-11-03 23:57:48]

miu@chromium.org changed reviewers:
+ thakis@chromium.org

[miu, 2014-11-03 23:58:26]

thakis: PTAL.  Simple crash fix.

[Nico, 2014-11-04 00:01:22]

lgtm

test would be nice

https://codereview.chromium.org/699013002/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

https://codereview.chromium.org/699013002/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:131: if (mediaIndicatorButton_) {
(nit: not needed, messages to nil are silently ignored)

[Nico, 2014-11-04 00:02:43]

Oh, and maybe you want to remove the __weak if it doesn't do what you want.
(I think it kind of does what you want, but only with arc, or if your
deployment target is new enough, or something like that)

On Mon, Nov 3, 2014 at 3:58 PM, <miu@chromium.org> wrote:

> Reviewers: Nico,
>
> Message:
> thakis: PTAL.  Simple crash fix.
>
> Description:
> [Cocoa] Crash fix, caused by recent tab indicator change.
>
> I misunderstood how ObjectiveC handled pointers declared with __weak.
> This change resolves the issue by nil'ing things out in
> [TabController dealloc], the standard solution.
>
> Introduced by: https://codereview.chromium.org/688523002/
>
> BUG=429605
>
> Please review this at https://codereview.chromium.org/699013002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+4, -0 lines):
>   M chrome/browser/ui/cocoa/tabs/tab_controller.mm
>
>
> Index: chrome/browser/ui/cocoa/tabs/tab_controller.mm
> diff --git a/chrome/browser/ui/cocoa/tabs/tab_controller.mm
> b/chrome/browser/ui/cocoa/tabs/tab_controller.mm
> index 08428b141c70620b975ee43b52b1873d4c4ef5a0..
> e994ad3d95fb05e9035ad3d40291d2fda199dae9 100644
> --- a/chrome/browser/ui/cocoa/tabs/tab_controller.mm
> +++ b/chrome/browser/ui/cocoa/tabs/tab_controller.mm
> @@ -128,6 +128,10 @@ class MenuDelegate : public
> ui::SimpleMenuModel::Delegate {
>  }
>
>  - (void)dealloc {
> +  if (mediaIndicatorButton_) {
> +    [mediaIndicatorButton_ setAnimationDoneTarget:nil withAction:nil];
> +    [mediaIndicatorButton_ setClickTarget:nil withAction:nil];
> +  }
>    [[NSNotificationCenter defaultCenter] removeObserver:self];
>    [[self tabView] setController:nil];
>    [super dealloc];
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2014-11-04 00:22:30]

Thanks for the quick turnaround on this.

https://codereview.chromium.org/699013002/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

https://codereview.chromium.org/699013002/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:131: if (mediaIndicatorButton_) {
On 2014/11/04 00:01:22, Nico wrote:
> (nit: not needed, messages to nil are silently ignored)

Done.

[miu, 2014-11-04 00:22:37]

The CQ bit was checked by miu@chromium.org

[Nico, 2014-11-04 00:22:57]

slgtm

[commit-bot: I haz the power, 2014-11-04 00:24:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699013002/20001

[miu, 2014-11-04 00:25:11]

On 2014/11/04 00:02:43, Nico wrote:
> Oh, and maybe you want to remove the __weak if it doesn't do what you want.
> (I think it kind of does what you want, but only with arc, or if your
> deployment target is new enough, or something like that)

I thought about that, but decided to leave it in for documentary purposes.  And,
as you point out, if ARC is enabled, it can only do exactly what I want.

[commit-bot: I haz the power, 2014-11-04 01:10:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-04 01:11:22]

Patchset 2 (id:??) landed as
https://crrev.com/b85fc6de11e5e162864d58a32da6a02a9a24ea3a
Cr-Commit-Position: refs/heads/master@{#302536}