1178984 - Crashes at nsMenuBarX::RemoveMenuAtIndex

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

These crashes have been around for a long time, in low numbers.  But currently they're the #2 Mac topcrasher on the 41 branch.  I suspect they have a very easy fix.

https://crash-stats.mozilla.com/search/?signature=RemoveMenuAtIndex&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 1

[Steven Michaud [:smichaud] (Retired)]

These crashes happen here, as we call nsMenuX::NativeMenuItem() on an invalid value referenced by mMenuArray[aIndex].  The invalid values are either NULL or 0x5a5a5a5a5a5a5a5a.

https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/widget/cocoa/nsMenuBarX.mm#l249

At first I thought we might be adding invalid values to mMenuArray in nsMenuBarX::InsertMenuAtIndex().  But we only have two calls to this method, and both insert a new nsMenuX object immediately after creating it, so I think this is impossible.

https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/widget/cocoa/nsMenuBarX.mm#l140
https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/widget/cocoa/nsMenuBarX.mm#l280

So looking at the top of nsMenuBarX::InsertMenuAtIndex(), it's pretty clear what the problem is:  We only check that aIndex < mMenuArray.Length() in debug builds (we only do an assertion).  Instead we should always return early, and call NS_ERROR.

Patch coming up shortly.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's my patch.

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b951427650d

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a2abd5d26c

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39a2abd5d26c

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8627938 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: Old bug
[User impact if declined]: Crashes, at high volume on aurora (41 branch)
[Describe test coverage new/current, TreeHerder]: Baked for several days on mozilla-central
[Risks and why]: Low risk -- patch just changes an assertion to an early return
[String/UUID change made/needed]: None

These crashes have disappeared from the firefox-2015-07-03-03-03-44-mozilla-central and later nightlies.  So I'm going to mark this bug VERIFIED.

I'll also request upload to aurora and beta.

These aren't a topcrasher on the 40 branch (beta).  But the patch is very simple and low risk, and we're quite early in the beta cycle.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8627938 [details] [diff] [review]
Fix

Verified crash fix. Given that the fix has been verified and that it is early in Beta, we'll take this in Beta as well as Aurora. Aurora+ Beta+

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/702fa333520f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c75740b01281