1131473 - crash in -[NativeMenuItemTarget menuItemHit:]

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Carsten Book [:Tomcat]]

This bug was filed from the Socorro interface and is 
report bp-0e883053-f5e7-4121-9075-a08152150210.
=============================================================

working on steps to reproduce and seems a low volume crash for mac so far.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Let's call this Cocoa widgets, since that's the code it happens in.

A quick search seems to show that this isn't new -- that it's been happening at low volume for quite a while.  That is if all crashes with this signature are actually the same crash:

https://crash-stats.mozilla.com/signature/?signature=-%5BNativeMenuItemTarget+menuItemHit%3A%5D

Comment 2

[Mats Palmgren (inactive)]

The volume for this signature is relatively high in v39 and v40, it's three times
higher in 40.0a2 than in 38.0.1 for example.

Several user comments mentions clicking "Firefox > About Firefox".

Comment 3

[Steven Michaud [:smichaud] (Retired)]

These crashes are strongly associated with an extension called µBlock or uBlock.  There are at least two distros, and multiple versions (perhaps multiple versions of each):

https://www.ublock.org/
https://addons.mozilla.org/en-US/firefox/addon/ublock/

https://github.com/chrisaljoudi/ublock
https://github.com/gorhill/uBlock

Comment 4

[Steven Michaud [:smichaud] (Retired)]

uBlock is sometimes identified using the following uuid:
{2b10c1c8-a11f-4bad-fe9c-1c11e82cac42}

Comment 5

[Steven Michaud [:smichaud] (Retired)]

I've been playing with various versions of uBlock, and still haven't found one with which I can reproduce this bug's crashes.  With all of them I've been using their default settings ... which may be the problem.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Hi Deathamns.

I'm needinfoing you on this bug because you're a major contributor to uBlock and already have a bugzilla.mozilla.org account.  As I mentioned above, these crashes have a very strong correlation with uBlock -- in 100% of the crash reports I looked at, some version of uBlock is installed.

No, I don't think uBlock is to blame here.  In fact I think that'd be impossible.  But uBlock's presence does seem to make these crashes more likely.  And, in order to fix this bug, we need to find out how to reproduce it.  I suspect someone who knows uBlock well has a much better chance of coming up with good (nearly 100% effective) steps-to-reproduce than I do.

As mentioned above, several comments (in the crash logs) mention clicking on Firefox : About.  One mentions doing that to check for updates.

* clicked on firefox-about
* Crash happened as I was opening the 'About Firefox' window to check for Firefox updates.
* Every, every single time I open Firefox Aurora and I click "About Firefox" the application crashes if there is a new update.

Other comments are visible here:
https://crash-stats.mozilla.com/report/list?signature=-[NativeMenuItemTarget%20menuItemHit%3A]#tab-comments

Thanks in advance for whatever help you can give us!

Comment 7

[Deathamns]

(In reply to Steven Michaud [:smichaud] from comment #6)

I'm not sure how could I help here. The issue (for me) doesn't even seem to be related to the extension.
Many users have this add-on installed, so it may be just a coincidence.
I saw a few reports where it was not present, for example:
https://crash-stats.mozilla.com/report/index/ec6ceebd-dd9e-46e1-99b1-b886f2150618#extensions
https://crash-stats.mozilla.com/report/index/7e84c2f0-47b3-4b5a-b974-87c4c2150618#extensions
https://crash-stats.mozilla.com/report/index/2ee5027a-b899-4e75-b167-50cf02150618#extensions
https://crash-stats.mozilla.com/report/index/a3b5d8e9-76f5-4946-a810-417ed2150618#extensions

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> Many users have this add-on installed, so it may be just a coincidence.

Yes, you show that's possible.  Thanks for the info.

I'm going to put this bug aside for the time being.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Looking at the assembly code for these crashes, it's likely that, once again, we're dealing with menuGroupOwner objects that have been deleted.  We've been plagued by this issue for years (as my old comment shows), without being able to find a good solution.  But just now I've thought of a potentially elegant workaround.  So I'll be working on this again for the next few days.

https://hg.mozilla.org/mozilla-central/annotate/ba43a48d3c52/widget/cocoa/nsMenuBarX.mm#l927

Comment 11

[Steven Michaud [:smichaud] (Retired)]

These are Mac topcrashers on the 43, 42 and 41 branches.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

This seems like a good way to work around the problem described in my old comment.  I don't know why I didn't think of it years ago :-)

Comment 13

[Stephen A Pohl [:spohl]]

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

Review of attachment 8653172 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55bea2798c68

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=13387028&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a4c5fbcca7

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I see the same crash manually (on quitting FF).  Sigh :-(

So this needs some more work ...

Comment 17

[Steven Michaud [:smichaud] (Retired)]

The problem is that I misinterpreted the Cocoa "create rule" (https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html).  I assumed I'd "own" any object created using either of the following:

-[[NSMutableSet alloc] initWithCapacity:]
+[NSMutableSet setWithCapacity:]

But that's wrong -- only methods 'whose name begins with “alloc”, “new”, “copy”, or “mutableCopy”' follow the "create rule".  So if I use the second method I need to also call "retain".

Yes, in some part of my mind I already knew this ... but it's been a while since I've had to deal with it explicitly :-(

New patch coming up.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1: What I will land

This should do the trick.  Carrying forward Stephen's r+.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8653712 [details] [diff] [review]
Fix rev1: What I will land

By the way, I double-checked (by adding an NSLog() statement to -[MenuItemInfo dealloc]) that this won't result in us leaking all MenuItemInfo objects -- it won't.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10592387dea4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10592387dea4

Comment 22

[Steven Michaud [:smichaud] (Retired)]

These crashes are gone in m-c nightly builds made since my patch landed.  I think that counts as verification.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8653712 [details] [diff] [review]
Fix rev1: What I will land

Approval Request Comment
[Feature/regressing bug #]: Very old bug
[User impact if declined]: Mac topcrasher will continue unfixed
[Describe test coverage new/current, TreeHerder]: Baked for five days on m-c -- crashes disappeared, no new problems reported
[Risks and why]: Low risk, simple patch
[String/UUID change made/needed]: None

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8653712 [details] [diff] [review]
Fix rev1: What I will land

Let's uplift to Aurora first and then to Beta in a few days once we know for sure that the crash has gone away without any unexpected fall outs. Aurora42+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fecd7b5233da

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8653712 [details] [diff] [review]
Fix rev1: What I will land

I looked at crash stats again and noticed that this does not occur on 42.0a2 since 09-03 when the fix was landed in m-a. I will take that as a good sign that this fix has helped. Let's uplift to Beta41.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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