477475 - Crash when KeyCue is running [@ nsMenuItemIconX::OnStopFrame]

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Tom Matthews]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6 Ubiquity/0.1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6 Ubiquity/0.1.5

See crash reports marked *TOM* : http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&version=Firefox%3A3.0.6&signature=libobjc.A.dylib%400x15688

This crash happens if the (left, but probably right as well) Command key is held down for a few seconds, no other key is held at the time and Firefox is the active window. This is likely an interaction with KeyCue (http://www.ergonis.com/products/keycue/) which binds to the Command key and displays a list of possible keyboard shortcuts after a user-specified delay.

I've yet to see any other application crash as a result of KeyCue so am guessing that Firefox isn't catching or handling an exception properly.

Reproducible: Always

Steps to Reproduce:
1. Have KeyCue installed (latest version will do).
2. Have Firefox installed (any sub-version of 3 will do).
3. Browse and then hold the Command key until you expect KeyCue to popup.
Actual Results:  
Firefox will crash just before the KeyCue popup is expected to appear.
KeyCue will continue to function as normal, but Firefox will be dead.

Expected Results:  
Firefox should lose focus as KeyCue pops up a window with the available keyboard shortcuts, then regain focus as the Command key is released.

crash-stats.mozilla.com crash IDs from today:
1ad8d116-bdc7-4ea2-a4ed-de08f2090208
adae1473-f695-46ec-908d-51b762090208
c522e187-82a5-4547-bfe8-947882090208
1368ccfe-06f2-416a-99e8-4b3782090208

Comment 1

[Steven Michaud [:smichaud] (Retired)]

The same crash stack is reported at bug 465623.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, but I cannot reproduce this crash with with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6 ID:2009011912 nor any other build, e.g. Shiretoko and Minefield

Tom, can you please try your steps with an upcoming FF3.1b3 build? You can find it here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/02/2009-02-09-02-mozilla-1.9.1/

Comment 3

[Tom Matthews]

Yes, I've just tried Shiretoko as advised and the crash occurs under the same conditions, might be a different stack trace though :
ID: 296e75b3-d15b-40b2-ad48-32b812090209
Signature: nsObjCExceptionLogAbort
http://crash-stats.mozilla.com/report/index/296e75b3-d15b-40b2-ad48-32b812090209?p=1

Comment 4

[Tom Matthews]

@Henrik Skupin : Do you also have KeyCue installed?

Comment 5

[Tom Matthews]

And again with Shiretoko, I just held down Command after I submitted the last crash report :
ID: 9d4427ab-7bdb-4d24-aee7-cb3452090209
Signature: libobjc.A.dylib@0x15688
http://crash-stats.mozilla.com/report/index/9d4427ab-7bdb-4d24-aee7-cb3452090209?p=1

Comment 6

[Tom Matthews]

A final update tonight :
ID: 2c6d971e-8389-4473-9a72-7f9cc2090209
Signature: libobjc.A.dylib@0x156a0
http://crash-stats.mozilla.com/report/index/2c6d971e-8389-4473-9a72-7f9cc2090209?p=1
I seem to be getting different crash signatures since using this latest nightly Shiretoko build? Yet the method of inducing them is the same, hold the Command key (it seems to occur when any number of pages are loaded as tabs, but more likely to occur when several are loaded i.e. >10)

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I hit the same crash. Thanks for the hint with several tabs:
bp-1576b21e-b8a0-4b63-8db2-a416e2090209

It would be still interesting to find the correct condition when this crash happens. It looks like it is not enough to have several tabs open. Tom, can you try to find out which steps have to be done with a fresh profile to get this crash? That would be really helpful. Thanks.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I can't reproduce this at all.  I have KeyCue 4.3 installed and
running, which is the current version.  I tested (on OS X 10.5.6) with
FF 3.0.6 and current Minefield and Shiretoko nightlies.  During most
of my tests, I had about 15 tabs open.

Tom, we need more information from you.

For example, I suspect this problem may be much more likely with
specific pages.  The next time you crash, try to remember exactly
which pages you had open (in the various tabs), and post them here.

Please also (as Henrik suggests) try with a fresh profile.

Comment 9

[Tom Matthews]

OK, here's what I have from this afternoons testing :

ID: 8ceb7b3e-5632-44e5-ac40-5b97c2090210
Signature: libobjc.A.dylib@0x15688
http://crash-stats.mozilla.com/report/index/8ceb7b3e-5632-44e5-ac40-5b97c2090210?p=1
Comment: I can't remember the exact sequence prior to this crash, was experimenting with multiple tabs and the Command key. 

ID: 00a5308a-fdf6-4b20-a5f6-93b762090210
Signature: nsObjCExceptionLogAbort
Remove Profiles directory from Library/Application Support/Firefox/
Start Shiretoko, choosing not to import anything.
Click the 'Latest Headlines' button in the bookmarks toolbar, choosing 'Open all in tabs', confirming when warned about possible slowness due to 31 tabs about to be opened.
Click a non active area in one of the opened tabs (it's a BBC News link : http://news.bbc.co.uk/2/hi/americas/7882143.stm), then hold Command and 'w' to close the tabs, after the last one closed, Shiretoko crashed.

ID: c4aada13-ff01-4007-a783-794f12090210
Signature: nsObjCExceptionLogAbort
Comment: Again, I can't remember the exact sequence, but I was experimenting with multiple tabs and the Command key.

ID: 0c65560f-bd50-46ac-ab99-1d4db2090210
Signature: libobjc.A.dylib@0x15688
http://crash-stats.mozilla.com/report/index/0c65560f-bd50-46ac-ab99-1d4db2090210?p=1
Remove Profiles directory from Library/Application Support/Firefox/
Start Shiretoko, choosing not to import anything.
Click the 'Latest Headlines' button in the bookmarks toolbar, choosing 'Open all in tabs', confirming when warned about possible slowness due to 31 tabs about to be opened.
Click a non active area in one of the opened BBC tabs, Shiretoko unresponsive with a 10 second or so delay, held Command key alone for approx 20 seconds, Shiretoko crashed.

I hope this is helpful.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #9)

The only one of these I can reproduce is the second
(00a5308a-fdf6-4b20-a5f6-93b762090210) ... which is actually a
different bug.  It's probably related to (if not a dup of) bug 435521.

I assume you're still running KeyCue.

Comment 11

[Tom Matthews]

(In reply to comment #7)
Not sure what else I can provide to help, Henrik have you been able to reproduce again?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I was, but it's not reliable reproducible for now. The crash happens when Shiretoko was open for a while. But after a restart and having the identical session open the crash doesn't occur. The given steps in comment 9 also don't crash Shiretoko with the targetted stack.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I hit the crash twice in a hour lately. I hadn't to hold the Command key. The first time it happened while closing a tab and the second time when trying to go backwards the history:

1. bp-a7f9b346-4af5-45b0-9234-906b92090211
2. bp-9ad90c4e-40fb-4d94-bac9-7efb42090212

You only have to start KeyCue and let it run in the background. For now I've deinstalled it again. Let's see if a similar crash occurs again.

Comment 14

[Tom Matthews]

(In reply to comment #13)
Yes, KeyCue always runs in the background, I really look forward to hearing if you can reproduce the error without it installed. I have another report from today, happened just as I was opening a new tab.
ID: b5ec0c14-9aa4-41b2-bd6f-36dda2090212
Signature: libobjc.A.dylib@0x15688
http://crash-stats.mozilla.com/report/index/b5ec0c14-9aa4-41b2-bd6f-36dda2090212?p=1

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I was able to. See bug 465623 for that. It also happened very sporadically. There are still no ways to cleanly reproduce it. :/

Comment 16

[Tom Matthews]

I think the bug re: shifted popups (465623) is unrelated to this one as out of the 26 crashes I've seen in the last week with signature libobjc.A.dylib@0x15688, I have not seen any shifted popups.
Having KeyCue installed has probably increased the likelyhood of hitting this bug as I'm more likely to be holding Command to view the possible keyboard shortcuts. The other crash reports with this signature appear to have comments mentioning holding Command-P to print, or Command-W to close a tab, and I've recently hit it trying Command-T to open a new tab.
Is there nothing useful so far in all these stack traces that can suggest why we're crashing?
Can we (I with some guidance) attach a debugger to the process to better see what is happening?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

> Is there nothing useful so far in all these stack traces that can
> suggest why we're crashing?

Actually, I can think of a patch that might fix your crashes ... or
that at least might change their location :-)

I'll post it (along with a tryserver build) later tonight or tomorrow
morning.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch for debugging

Here's my debugging patch.  And here's a tryserver build:

https://build.mozilla.org/tryserver-builds/2009-02-12_17:[e-mail]-bugzilla477475-debug/[e-mail]

Tom, please test the tryserver build.  Let us know if it gets rid of
your crashes, or if it (somehow) changes the crashlogs.

Also look for messages with "***SHIT!***" in them in your system
console ... and let us know which ones you see :-)  Do this even if
you see no crashes.

Comment 19

[Tom Matthews]

Thanks for the patched tryserver build, I can't get it to submit or capture a breakpad crash report, but CrashReporter is capturing some of the stack :
http://pastebin.com/m12b956d7

Here are the console logs for the last two crashes. Sadly no ********** messages.
13/02/2009 13:19:48 ReportCrash[6195] Formulating crash report for process firefox-bin[6182] 
13/02/2009 13:19:50 ReportCrash[6195] Saved crashreport to /Users/tom/Library/Logs/CrashReporter/firefox-bin_2009-02-13-131942_habitat.crash using uid: 501 gid: 20, euid: 501 egid: 20 
13/02/2009 13:19:50 ReportCrash[6195] Saved crashreport to /Users/tom/Library/Logs/CrashReporter/firefox-bin_2009-02-13-131942_habitat.crash using uid: 501 gid: 20, euid: 501 egid: 20 
13/02/2009 13:30:42 ReportCrash[6212] Formulating crash report for process firefox-bin[6198] 
13/02/2009 13:30:43 ReportCrash[6212] Saved crashreport to /Users/tom/Library/Logs/CrashReporter/firefox-bin_2009-02-13-133040_habitat.crash using uid: 501 gid: 20, euid: 501 egid: 20 
13/02/2009 13:30:42 com.apple.launchd[167] ([0x0-0x25d25d].org.mozilla.firefox[6198]) Exited abnormally: Segmentation fault 
13/02/2009 13:30:42 com.apple.launchd[167] ([0x0-0x25d25d].org.mozilla.firefox[6198]) Exited abnormally: Segmentation fault 
13/02/2009 13:31:22 SubmitReport[6222] Submitted compressed crash report for firefox-bin

Comment 20

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #19)

Thanks for testing my tryserver build.  Too bad it didn't fix the
crashes ... but your tests (and the fact that you didn't see any
***shit*** messages) have ruled out some possible causes.  I'll submit
another patch (and tryserver build) later today -- for more tests.

> I can't get it to submit or capture a breakpad crash report

I'd forgotten about this.  I'll try to enable Breakpad in my next
tryserver build ... though I'm not sure I'll succeed.

> CrashReporter is capturing some of the stack :
> http://pastebin.com/m12b956d7

I'm afraid these stacks aren't worth much -- installable packages
always have their debug symbols stripped, so all the Mozilla-specific
symbols in these stacks are spurious.  But at least the crashes still
seem to be the same (the symbol at the very stop of the stack *isn't*
spurious), and we now have slightly more information -- the fact that
they happen during a call to objc_msgSend (which helps narrow things
down).

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Knowing the crashes happen in objc_msgSend narrows the focus *a lot*.
So I'm reasonably hopeful this patch will fix them.

Here's the tryserver build:

https://build.mozilla.org/tryserver-builds/2009-02-13_09:[e-mail]-bugzilla477475/[e-mail]

Please try it, Tom, and let us know your results.

Comment 22

[Tom Matthews]

Looking good so far, no crashes whatsoever!
Hopefully this isn't too premature, but thank you very much!

Comment 23

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #22)

Glad to hear it!  Please let us know in a few days if the crashes "stay fixed".

I'll start the review process on the strength of your current results.

Comment 24

[Josh Aas]

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

Steven - can you give an explanation for this patch?

Comment 25

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #24)

Beyond the obvious (that nsMenuItemIconX::OnStopFrame()) can send a
message to a deleted mNativeMenuItem object), I'm not really sure.

I'll look into this further tomorrow (when I'm back to work).

I suspect this patch may also fix other bugs (like bug 435521, and
what I talk about in comment #10 above).  But I find these bugs so
hard to reproduce that it's difficult to be sure.

Tom, could you try your second STR from comment #9 with my tryserver
build from comment #21?  And could you also try to reproduce bug
435521 with it?  (See particularly bug 435521 comment #0, bug 435521
comment #8 and bug 435521 comment #18.)

As a control, you should also try to reproduce these bugs with a
current Minefield nightly (not Shiretoko or GranParadiso) -- tryserver
builds are based on current trunk (mozilla-central, Minefield) code.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #24)

> I suspect this patch may also fix other bugs (like bug 435521, and
> what I talk about in comment #10 above).  But I find these bugs so
> hard to reproduce that it's difficult to be sure.

I've found out my patch doesn't fix bug 435521:  I ran my tryserver
build while my OS X 10.5.6 box was under heavy load, and there were
about 85 items in the download window -- and I did manage to get it to
crash.

So the explanation of my patch is that it prevents
nsMenuItemIconX::OnStopFrame() from occasionally sending messages to a
deleted mNativeMenuItem object.

Tom, you needn't bother with the tests I asked you to do in comment
#25.  But I'd still like to hear your results after a few days of
testing for this bug's crashes.

Comment 27

[Josh Aas]

Why is the weak reference not good enough? I want to avoid working around a potential bug in KeyCue, perhaps it is incorrectly over-releasing a menu item? Who is releasing the menu item that we crash trying to send a message to, in what order?

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> Who is releasing the menu item that we crash trying to send a message to, in
> what order?

I can't know this, since I can't reproduce the bug.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> I want to avoid working around a potential bug in KeyCue

I doubt this is a bug in KeyCue, since Henrik can reproduce the same crash without KeyCue (at bug 465623).

Comment 30

[Josh Aas]

Attachment: native item autorelease fix v1.0

In nsMenuItemX::Create we create a native menu item and then a nsMenuItemIconX object with a weak reference to the native menu item.

In nsMenuItemX::~nsMenuItemX we autorelease native menu item and then the nsMenuItemIconX member object gets its destructor called, where it calls "CancelAndForgetObserver". The autorelease on the native menu item means that the native menu item should stay alive until after the nsMenuItemIconX object has its destructor called.

The situation is almost the same for nsMenuX, but there we release the native menu item instead of autoreleasing it. That means there is a period of time in which the nsMenuX's nsMenuItemIconX object would contain a bad weak reference - after the release in the nsMenuX destructor and before the nsMenuItemIconX member object's destructor runs. This patch fixes that and here is a tryserver build:

https://build.mozilla.org/tryserver-builds/2009-02-17_19:[e-mail]-1234927078/[e-mail]

If this does not fix the problem then it seems to me there are only a couple explanations for this crash. One option is that the autorelease pool containing the native menu item somehow releases its objects between the time we autorelease the native menu item and the nsMenuItemIconX object receives its last observer notification. The other option is that there is a bug in our image notifier code that causes it to call OnStopFrame after CancelAndForgetObserver has been called on the observer.

Either way, making the nsMenuItemIconX's reference to the native menu item strong doesn't appear to be the right fix. It may be the right fix in the end, but I need a better explanation. The nsMenuItemIconX's lifetime should be be confined within the lifetime of an nsMenuX or nsMenuItemX object, and those two objects should be guaranteed to have a valid native menu item at all times. Making the icon's native menu item reference strong makes it harder to notice when we break those lifetime assumptions.

Comment 31

[Josh Aas]

BTW, I'm relying on looking over the code because I can't reproduce the crash with the latest version of KeyCue.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Josh:

I can't reproduce this bug, but I *can* (with difficulty) reproduce bug 435521.  And it seems that your patch (unlike mine) also fixes that bug.  In bug 435521 comment #20 I've asked Marcia and Stephen to also try your tryserver build.

Tom:

Please try Josh's tryserver build from comment #30, and let us know your results.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I hit this crash several times today. More while release testing FF3.0.7. I'll try to find a reproducible str later.

Latest crash: bp-655a0813-0aa6-449e-ae4c-df0c42090220

Comment 34

[Josh Aas]

Comment on attachment 362843 [details] [diff] [review]
native item autorelease fix v1.0

I think my analysis shows we have a problem that this patch fixes, whether or not it does fix this bug in particular. I think we should take it. I'll add some comments about lifetime expectancies before landing.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 362843 [details] [diff] [review]
native item autorelease fix v1.0

I suspect you're right, Josh, that this bug is a timing bug, and that
your patch will fix it.  I'm pretty sure the same is true of bug
435521 (which is why your patch fixes that bug).

If Tom still reports crashes, you can easily combine my patch with
yours.  This would (I think) mean that your patch doesn't (by itself)
completely fix the timing problem.  But it surely much alleviates it,
and thereby reduces the strength of your objections to my patch.  (In
other words, if my patch needs to be taken together with your patch,
the "extra" time that mNativeMenuItem stays alive will surely be much
less than with my patch alone.)

Comment 36

[Josh Aas]

Attachment: native item autorelease fix v1.1

same thing with lifetime comments added

Comment 37

[Josh Aas]

pushed "native item autorelease fix v1.1" to mozilla-central

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

I'm not closing out this bug until we get fix confirmation from those who can reproduce this.

Comment 38

[Tom Matthews]

The build from Comment#30 seems to fix it as well. At least in the last few hours of testing I can't reproduce the bug anymore.
Many thanks again for you chaps attention on this one.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

You're most welcome.  And thanks for testing.

Comment 40

[Tom Matthews]

Typical, I get a crash just after posting that, I just closed the tryserver build with Command-Q, but again this build doesn't seem to have breakpad enabled so only Crash Reporter caught it. I can't tell if it's related, I hope not :
http://pastebin.com/m1d073131

Comment 41

[Steven Michaud [:smichaud] (Retired)]

> http://pastebin.com/m1d073131

This is a different crash, and presumably unrelated -- there's no objc_msgSend at the top.  And as with the crash you posted at comment #19, all the Mozilla-specific symbols are spurious, so it's basically useless.

If you see the same kinds of crashes with regular builds (nightlies, betas, releases), open a new bug, post Breakpad ids, and try to give some kind of steps-to-reproduce.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

> http://pastebin.com/m1d073131

Also, this is a null-pointer deference, and not a reference to a deleted object.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Tom, Josh's patch will be in tomorrow's Minefield nightly.

Try downloading and testing with that.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, there is no need to hold down the Cmd key. I was able to crash Shiretoko right after KeyCue was activated. Updating summary. I'll run the tryserver build to see if it fixes the crash for me too.

Comment 45

[Tom Matthews]

(In reply to comment #43)
> Tom, Josh's patch will be in tomorrow's Minefield nightly.
> 
> Try downloading and testing with that.

Sorry, I'm being a little slow, ftp.mozilla.org/pub/mozilla.org/firefox is a confusing directory.

Hope this is the latest nightly Minefield build as thats the one I'm trying now :
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-3.2a1pre.en-US.mac.dmg

Comment 46

[Tom Matthews]

I'm been using :
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090222 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090222020446

And have hit these two bugs under the 'holding command key' scenario:
ID: 66dca809-fafa-4fd8-b321-372002090222
Signature: libobjc.A.dylib@0x156a0

ID: d355d171-0d04-4178-8672-169e22090222
Signature: libobjc.A.dylib@0x156a0

Will go back to using Stevens tryserver build from Comment#21 for the time being.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #46)

These crash ids are both for this bug's crash.  So Josh's patch
apparently doesn't fully resolve this bug's crash.

(In reply to comment #45)

You're right -- the naming scheme we use for our nightlies is very
confusing.  And the README file doesn't provide much help.

But the link you found is the right one.

"Minefield", "trunk" and "mozilla-central" all refer to the same
thing.

Likewise with "Firefox 3.1", "Shiretoko" and "1.9.1 branch".

Likewise with "Firefox 3.0.X", "GranParadiso" and "1.9.0 branch".

(In reply to comment #46)

Your user-agent string is interesting.  It appears to show that you've
installed the Ubiquity extension ... though I don't exactly know how
Ubiquity can override the browser's user-agent string.

I wonder if using Ubiquity makes this bug more likely to happen.  What
other extensions are you using?

Comment 48

[Tom Matthews]

(In reply to comment #47) 
> I wonder if using Ubiquity makes this bug more likely to happen.  What
> other extensions are you using?

Adblock Plus 1.0.1
Firefox PDF Plugin for Mac OS X 1.0.1 [DISABLED]
Nightly Tester Tools 2.0.2
Sage-Too 1.0.1
Tamper Data 10.1.0 [DISABLED]
Ubiquity 0.1.5 [DISABLED]
User Agent Switcher 0.6.11 [DISABLED]

Comment 49

[Tom Matthews]

Still hitting the bug even with the most recent trunk (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090224 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090224020633) :
ID: f94a9339-1838-4dfd-9fa8-2a9c82090225
Signature: libobjc.A.dylib@0x15688

Were both Steven and Josh's patches combined and and put into trunk?

Comment 50

[Josh Aas]

"Works around a problem with Firefox that caused KeyCue to fail after closing the initial window of Firefox."

This is from the release notes of KeyCue 4.4, which leads me to believe they were holding a reference to the main Cocoa menu bar. We have a different menu bar for each window, unlike most apps, but that is totally "legal" and not a problem. That could very well explain this crash, making it KeyCue's fault not ours (they're messing with our native menu object). Can anyone reproduce this with KeyCue 4.4+?

Comment 51

[Tom Matthews]

http://crash-stats.mozilla.com/report/index/4a7ac670-901b-4e08-bf79-aa15e2090323?p=1
With KeyCue 4.4

Comment 52

[Tom Matthews]

So, Stevens tryserver build from comment #21 seemed to fix the issue, did that patch never end up being commited to the trunk? Just curious now that Josh is withdrawing?

Comment 53

[Jan]

Hi guys,

today I exchanged emails with the KeyCue support team concerning this Firefox crashing bug and they offered me to support your bugfixing efforts if still needed. Their support member gave me the following contact information and asked me to join them with you - so here they are:

KeyCue Support = Günther Blaschek
Ergonis Software GmbH
Softwarepark 37, 4232 Hagenberg, Austria
keycue-support(at)ergonis(dot)com - www.ergonis.com

Thx and good luck,
Jan

Comment 54

[Steven Michaud [:smichaud] (Retired)]

Attachment: Update of comment #21 patch for current code

Here's an update of my patch from comment #21.

And here's a tryserver build:

https://build.mozilla.org/tryserver-builds/2009-05-11_18:[e-mail]-bugzilla477475-v20/[e-mail]

This should fix not only this bug, but also bug 465623, bug 492045 and
bug 457316.

For more info see bug 492045.

Tom, please test with this tryserver build.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Here's a 1.9.1-branch tryserver build made with my comment #54 patch:

https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla477475-v20-191/bugzilla477475-v20-191-macosx.dmg

Comment 56

[Jan]

Hi Steven, 

I just finished testing your tryserver build from #55 for half an hour on my MBP 10.5.7 (KeyCue build 5105) and encountered no FF crashes despite an intensive use of command key based KeyCue trigger actions. 
Sometimes, pressing the command key unexpectedly failed to trigger KeyCue at all and I had to klick somewhere in the then active tab and try again to successfully bring up the KeyCue menue. But that's fine, I'm very happy with your build and fix for this annoying bug.

Thank you very much!
Jan

Comment 57

[Tom Matthews]

Hi Steven, been using the build from comment #54 for a while now and am so happy to report no crashes whatsoever.
I'll be giving the 1.9.1 branch tryserver build a go from today.
Many thanks for your work on this bug :)

Comment 58

[Jan]

(In reply to comment #55)
 
Hi Steven again,
after ten days of intense testing I'm still free of trouble with your build. Sadly, I'm not authorized to give your patch a go or a basic review+ .
Can we help somehow, to get your patch into the official FF branch?

Thx again,
Jan

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Josh, is the latest patch a way you want to see to fix this problem? As it sounds it works fine.

Comment 60

[Josh Aas]

Comment on attachment 376933 [details] [diff] [review]
Update of comment #21 patch for current code

I'm not likely to r+ this or any other patch until somebody can explain *why* their patch works (Steven says "I'm not really sure" in comment #25). This is papering over the bug, not fixing it. We could never release anything and then we'd never have any freed-object crashes!

So long as a menu item or submenu exists, and within that scope its icon, there must be a valid native menu item. That is the invariant. I don't want patches that break it, and I don't patches that paper over bugs that break it (unless the situation is critical, which this is not).

Comment 61

[Martijn Wargers (dead)]

(In reply to comment #60)
> that break it, and I don't patches that paper over bugs that break it (unless
> the situation is critical, which this is not).

This is a crash bug, so it is critical, no?

Comment 62

[Josh Aas]

(In reply to comment #61)
> This is a crash bug, so it is critical, no?

Not critical enough to take (probably incorrect) patches we can't explain. As far as I know only a small subset of users hit this, those with KeyCue installed. Not even all KeyCue users hit this (I couldn't reproduce with KeyCue, iirc neither can Steven). We don't even have any evidence that this isn't a KeyCue bug because nobody here knows what is going on with this crash.

I suspect this is a KeyCue problem though I don't know for sure because I can't reproduce it to debug.

Comment 63

[Josh Aas]

I emailed Ergonis (makers of KeyCue) about this, maybe they can provide some insight into KeyCue's interaction with Firefox.

Comment 64

[Josh Aas]

Got some information from the KeyCue folks:

"We have been chasing this issue for months, and have already narrowed it down to the Bookmarks and History menus. At least, we have a few users who run tests for us."

"The problem is that we have not been able to reproduce this problem with Firefox, but that is probably because we use Firefox only occasionally. Even those users who run Firefox regularly cannot reliably reproduce the problem on demand."

"KeyCue does not mess around within the memory space of Firefox. In particular, this means that KeyCue does not even have a chance to modify the retain counts. KeyCue uses the accessibility API of Mac OS X to scan all menus and their menu items (including submenus), searching for items that have keyboard shortcuts. I don't know how Firefox implements accessibility support, but from what users tell us, it could be that Firefox releases objects in the course of such an inquiry."

"KeyCue has the ability to cache the information acquired about menus in order to speed up subsequent invocations. However, KeyCue does not cache references to accessibility objects across invocations for Firefox (we already have turned this off specifically for Firefox, hoping it would help)."

"However, KeyCue does keep references to AX objects for menus and menu items around when it displays the shortcut table. We need to do this because users can click items in the shortcut table in order to execute the menu commands."

Comment 65

[Steven Michaud [:smichaud] (Retired)]

Tom and Jan:

Sorry to bug you again (and sorry that this seemingly simple bug is taking so very long to resolve).  But I've got another patch that should fix this bug, together with a new tryserver build (at bug 499600 comment #9).

This patch doesn't make mNativeMenuItem a strong pointer, so Josh may like it better.

Comment 66

[Jan]

(In reply to comment #65): latest tryserver build

Hi Steven, 
thanks a lot, for not giving up on us KeyCue users!
I tested your tryserver build for about a week now and had only non-KeyCue related crashes (Adobe Flash hit me twice). So this patch seems to fix KeyCue's FF crashing quite well. However, during some FF-sessions I was unable to trigger KeyCue (by holding the command key) - but after a system restart it works flawless.

I hope your build will get a positive superreview soon,
thanks again,

Jan

Comment 67

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This should have been fixed by the patch on bug 499600. If anyone who was
able to see the crash can verify with a recent Minefield build that would be
nice.

Comment 68

[Steven Michaud [:smichaud] (Retired)]

My (new) patch for bug 499600 is in current Minefield (aka mozilla-central) nightlies, available from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/.

Tom and Jan, please test it and let us know your results.

Comment 69

[Jan]

Voila,

no crashes for me after another week of intense testing. I'm happy with your patch from #68, Steven, and will keep this Minefield build for now.

Thanks a lot,
Jan

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #69)
> no crashes for me after another week of intense testing. I'm happy with your
> patch from #68, Steven, and will keep this Minefield build for now.

Jan, does it only apply to Minefield or do you also have tested a Firefox 3.5.3pre build? Marking verified fixed on trunk meanwhile.

Comment 72

[Jan]

(In reply to comment #71)
Hi Henrik, no, I didn't. But if the fix has also been implemented there, I will do that to regain the functions of more of my addons.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes it is. You can grab a build from the location below. It would be great if you can give us feedback.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/

Comment 74

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

I'd like to find a way to verify this as "fixed" for 1.9.1.3. Henrik, can you try with the 1.9.1 nightly from 8/18?

Comment 75

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Al, it's hard to trigger this crash. I was able in former times but we should trust Jan here. I hope he will come back to us soon with his results.

Comment 76

[Jan]

Hi guys,

I gave yesterday's Shiretoko - the FF 3.5.3.pre nightly build, Henry linked me to - a two day try and keyCueed it grimly. No KeyCue related crash occured in those situations, where I was used to face them on a regular basis - indeed, no crash at all occured. I'm as happy with this version as I was with the Minefield build.

Thanks again, I'm happy, if I could help you guys,
Jan

Comment 77

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That sounds great! Thanks Jan for helping out. We appreciate it. Marking verified1.9.1.