Fix crash in ProfileMenuController::validateMenuItem.

Fix crash in -[ProfileMenuController validateMenuItem:].

This CL also instruments the function to better understand which edge case
triggers the issue in the first place.

BUG=471586
Committed: https://crrev.com/8c1b23970f6d8efe9e68f6dfdadbbf7b0e833417
Cr-Commit-Position: refs/heads/master@{#326835}

[anthonyvd, 2015-04-22 15:38:01]

anthonyvd@chromium.org changed reviewers:
+ asvitkine@chromium.org, noms@chromium.org

[anthonyvd, 2015-04-22 15:38:02]

Hi asvitkine@, could you please take a look at this change?

I believe this should fix the crashes in crbug.com/471586 but it also adds
instrumentation to better understand the edge cases that trigger it to
eventually fix the root cause. I'd also like your input if you can think of a
better way to gather those signals.

noms@ if you could just take a quick glance at this change and see if it makes
sense I'd really appreciate.

Thanks a lot to both of you for your time :)

[Alexei Svitkine (slow), 2015-04-22 16:55:25]

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm (right):

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:30: enum
ValidateMenuItemSelector {
Don't indent within namespace. Add a comment that this is used in an UMA macro
and that entries shouldn't be renumbered or removed.

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:195:
UMA_HISTOGRAM_ENUMERATION("ValidateMenuItemInvalidIndexSelector",
Histograms use a dotted naming notation - e.g. Foo.Bar.Baz. Please update these
names to use that, nesting them under some existing category (Profiles?).

[anthonyvd, 2015-04-23 14:49:01]

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm (right):

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:30: enum
ValidateMenuItemSelector {
On 2015/04/22 16:55:25, Alexei Svitkine wrote:
> Don't indent within namespace. Add a comment that this is used in an UMA macro
> and that entries shouldn't be renumbered or removed.

Done.

https://codereview.chromium.org/1054113004/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:195:
UMA_HISTOGRAM_ENUMERATION("ValidateMenuItemInvalidIndexSelector",
On 2015/04/22 16:55:25, Alexei Svitkine wrote:
> Histograms use a dotted naming notation - e.g. Foo.Bar.Baz. Please update
these
> names to use that, nesting them under some existing category (Profiles?).

Done.

[Alexei Svitkine (slow), 2015-04-23 15:02:54]

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm (right):

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:39: }
Nit: // namespace

Also, put empty lines around the enum definition.

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:187:
ValidateMenuItemSelector current_selector = UNKNOWN_SELECTOR;
Use camelCase.

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30718: +    The selector associated with
the menu item when validateMenuItem in the
Nit: For Obj-C methods, the convention is to refer to them with a leading "-",
i.e. "-validateMenuItem". Please fix throughout.

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63297: +  <int value="4"
label="switchToProfileFromMenu"/>
Why are these two the same?

Also, these are human readable names - so maybe change to explain things more.

[anthonyvd, 2015-04-23 19:04:57]

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm (right):

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:39: }
On 2015/04/23 15:02:54, Alexei Svitkine wrote:
> Nit: // namespace
> 
> Also, put empty lines around the enum definition.

Done.

https://codereview.chromium.org/1054113004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller.mm:187:
ValidateMenuItemSelector current_selector = UNKNOWN_SELECTOR;
On 2015/04/23 15:02:54, Alexei Svitkine wrote:
> Use camelCase.

Done.

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30718: +    The selector associated with
the menu item when validateMenuItem in the
On 2015/04/23 15:02:54, Alexei Svitkine wrote:
> Nit: For Obj-C methods, the convention is to refer to them with a leading "-",
> i.e. "-validateMenuItem". Please fix throughout.

Done.

https://codereview.chromium.org/1054113004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:63297: +  <int value="4"
label="switchToProfileFromMenu"/>
On 2015/04/23 15:02:54, Alexei Svitkine wrote:
> Why are these two the same?
> 
> Also, these are human readable names - so maybe change to explain things more.

Copy-paste error, thanks.

Done.

[Alexei Svitkine (slow), 2015-04-23 20:47:13]

lgtm

[noms (inactive), 2015-04-23 20:50:38]

lgtm. 

The only thing I'd try maybe try to confirm is whether the active profile is
Guest when this happens. Otherwise, if the profile isn't guest, it's some sort
of zombie profile, and Chrome is in a really, really suspicious state. 

Maybe add a DCHECK in that `if` statement?

[anthonyvd, 2015-04-24 14:27:12]

On 2015/04/23 20:50:38, noms (slow. ping if too slow) wrote:
> lgtm. 
> 
> The only thing I'd try maybe try to confirm is whether the active profile is
> Guest when this happens. Otherwise, if the profile isn't guest, it's some sort
> of zombie profile, and Chrome is in a really, really suspicious state. 
> 
> Maybe add a DCHECK in that `if` statement?

Well there's a few code paths that appear to allow
AvatarMenu::GetActiveProfileIndex() to return 0 when
AvatarMenu::GetNumberOfItems() is 0 which would be one of the cases that branch
inside that if statement. I'm not sure the assumption that we're in a guest
profile necessarily holds.

Since the purpose of that code is to figure out which edge case is being hit,
how do you feel about not making that DCHECK considering I'm logging the
guest/not guest state in UMA?

[noms (inactive), 2015-04-24 16:46:45]

On 2015/04/24 14:27:12, anthonyvd wrote:
> On 2015/04/23 20:50:38, noms (slow. ping if too slow) wrote:
> > lgtm. 
> > 
> > The only thing I'd try maybe try to confirm is whether the active profile is
> > Guest when this happens. Otherwise, if the profile isn't guest, it's some
sort
> > of zombie profile, and Chrome is in a really, really suspicious state. 
> > 
> > Maybe add a DCHECK in that `if` statement?
> 
> Well there's a few code paths that appear to allow
> AvatarMenu::GetActiveProfileIndex() to return 0 when
> AvatarMenu::GetNumberOfItems() is 0 which would be one of the cases that
branch
> inside that if statement. I'm not sure the assumption that we're in a guest
> profile necessarily holds.
> 
> Since the purpose of that code is to figure out which edge case is being hit,
> how do you feel about not making that DCHECK considering I'm logging the
> guest/not guest state in UMA?

Ah I missed that it was going in UMA. Sounds good!

[anthonyvd, 2015-04-24 17:05:26]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-04-24 17:05:39]

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

[commit-bot: I haz the power, 2015-04-24 18:06:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 18:07:45]

Patchset 3 (id:??) landed as
https://crrev.com/8c1b23970f6d8efe9e68f6dfdadbbf7b0e833417
Cr-Commit-Position: refs/heads/master@{#326835}