Closed
Bug 1178984
Opened 10 years ago
Closed 10 years ago
Crashes at nsMenuBarX::RemoveMenuAtIndex
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla42
People
(Reporter: smichaud, Assigned: smichaud)
Details
(Keywords: crash, topcrash-mac)
Crash Data
Attachments
(1 file)
1.04 KB,
patch
|
spohl
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Crash Signature: [@ nsMenuBarX::RemoveMenuAtIndex(unsigned int) ]
Keywords: crash,
topcrash-mac
Assignee | ||
Comment 2•10 years ago
|
||
Here's my patch.
I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b951427650d
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8627938 -
Flags: review?(spohl.mozilla.bugs)
Updated•10 years ago
|
Attachment #8627938 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 4•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #8627938 -
Flags: approval-mozilla-beta?
Attachment #8627938 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 6•10 years ago
|
||
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+
Attachment #8627938 -
Flags: approval-mozilla-beta?
Attachment #8627938 -
Flags: approval-mozilla-beta+
Attachment #8627938 -
Flags: approval-mozilla-aurora?
Attachment #8627938 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•