532585 - Crash [@ nsMenuX::MyMenuEventHandler]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Ben Frisch]

While browsing the crash database I noticed that http://crash-stats.mozilla.com/report/list?product=Firefox&platform=mac&query_search=signature&query_type=exact&query=MyMenuEventHandler&date=&range_value=1&range_unit=weeks&do_query=1&signature=MyMenuEventHandler lists 13 crashes in MyMenuEventHandler in 3.5 and 3.6.  

The crash should be mitigated by the earlier check of the visible item count, but while I couldn't reproduce the crash myself, even with various configurations in the menu editor extension, it seems that GetVisibleItemAt is probably returning NULL even if aPos < what GetVisibleItemCount would return, as GetVisibleItemAt queries the content node's visibility state during the loop to check visibility.  As such, it's possible that the node visibility could have changed in the content node, but the menu item could not have received the notification yet, so, it could still be clickable and counted as being visible (probably exacerbated by the combination of the Mac OS and the Gecko event loops).  Furthermore, all of the users who crashed do have the menu editor extension installed, so, it is very likely that they would make some nodes invisible, thus requiring the check of each menu node's visibility.

Patch that should fix it is coming up shortly.

Comment 1

[Ben Frisch]

Attachment: Crash Fix v.1

Comment 2

[Ben Frisch]

Comment on attachment 415803 [details] [diff] [review]
Crash Fix v.1

This should fix the crash by checking if targetMenu->GetVisibleItemAt returns null and returning eventNotHandledErr if it does.  targetMenu->GetVisibileItemAt checks the visible item count initially anyway.  I am still having trouble reproducing the crash even once, but this seems to be a simple and logical fix for the crash reports which all occurred on the same "if (target->MenuObjectType() == eMenuItemObjectType) {" line of code.

Comment 3

[Josh Aas]

Doesn't look like we're crashing accessing NULL in most cases, we're crashing on a bad address which is not 0x0. If that is true then a NULL check won't usually help.

Comment 4

[Josh Aas]

Comment on attachment 415803 [details] [diff] [review]
Crash Fix v.1

I think this patch makes things safer and more efficient whether or not it actually fixes the crash. We should take it.

Comment 5

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/df6c991d388e

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 415803 [details] [diff] [review]
Crash Fix v.1

Another that we should take in 1.9.2.3, really. Simple fix, obviously safe.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 415803 [details] [diff] [review]
Crash Fix v.1

Approved for 1.9.2.4, a=dveditz for release-drivers

Josh: can you check this in?

Comment 8

[Josh Aas]

pushed to mozilla-1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f22f55466394

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Is there anything for QA to verify here for 1.9.2? It doesn't look like there is a reproducible case.