Last Comment Bug 728964 - Crash [@ -[NativeMenuItemTarget menuItemHit:] ] when 'About Thunderbird' is clicked
: Crash [@ -[NativeMenuItemTarget menuItemHit:] ] when 'About Thunderbird' is c...
Status: RESOLVED FIXED
: crash, reproducible
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 11 Branch
: x86 Mac OS X
: -- critical (vote)
: Thunderbird 13.0
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
http://crash-stats.mozilla.com/report...
Depends on: 738074
Blocks: 843551 tb-tabsontop
  Show dependency treegraph
 
Reported: 2012-02-20 15:18 PST by Kyle Kalstabakken
Modified: 2013-05-08 05:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
-
[NativeMenuItemTarget menuItemHit:]
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Gdb crash stack (today's trunk code) (37.05 KB, text/plain)
2012-02-21 14:08 PST, Steven Michaud [:smichaud] (Retired)
no flags Details
Proposed fix (694 bytes, patch)
2012-02-28 12:33 PST, Steven Michaud [:smichaud] (Retired)
mconley: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Kyle Kalstabakken 2012-02-20 15:18:44 PST
Steps to reproduce (for me at least):
1. Open Thunderbird
2. Close the Thunderbird window (but do not exit the program)
3. Open a new Thunderbird window (e.g., click on the dock icon)
4. Click on Thunderbird->About Thunderbird

At this point, one of two things happen:
1. The menu closes, but nothing else happens OR
2. Thunderbird crashes.

If #1 happens, the program has always crashed after I clicked About Thunderbird a second time.

Usually I will be asked to submit a crash report, but occasionally I will just get an OS error message saying Thunderbird closed unexpectedly.

I've included a link to me latest crash report in the URL field.

Maybe related, maybe a different bug:
After closing the Thunderbird window (so no windows are open, but the program is still running), clicking About Thunderbird closes the menu, but otherwise does absolutely nothing.
Comment 1 :aceman 2012-02-21 03:59:52 PST
Can you retrieve the crash report id?
Instructions for Thunderbird at https://support.mozillamessaging.com/en-US/kb/Mozilla-Crash-Reporter#w_viewing-crash-reports
Comment 2 Kyle Kalstabakken 2012-02-21 07:07:07 PST
bp-ff929447-1951-47b7-8b9c-08e8d2120220
Comment 3 :aceman 2012-02-21 07:26:30 PST
Thanks.
Comment 4 Steven Michaud [:smichaud] (Retired) 2012-02-21 09:18:39 PST
This is probably a dup of bug 670914 (reported for Thunderbird).  It also looks related to bug 704866 (reported for Firefox).  But the latter really does seem to be "fixed" -- there are very few of these crashes in Firefox since my patch for bug 704866 landed on the 11 branch.

As I said at bug 670914 comment #5, it's really hard to make any progress on these crashes if we're not able to reproduce them.  Can anyone else besides the reporter reproduce his STR from comment #0?
Comment 5 Steven Michaud [:smichaud] (Retired) 2012-02-21 09:24:49 PST
Hooray!  I can repro with today's comm-central nightly!

I'll get to work on this as soon as I can (this week or next).
Comment 6 Steven Michaud [:smichaud] (Retired) 2012-02-21 09:29:04 PST
> This is probably a dup of bug 670914 (reported for Thunderbird).

Please don't dup this bug to bug 670914.  Only this bug has true STR.  So it's more convenient to work on it here.
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-02-21 14:08:01 PST
Created attachment 599349 [details]
Gdb crash stack (today's trunk code)
Comment 8 Steven Michaud [:smichaud] (Retired) 2012-02-27 14:55:24 PST
I've been working on this bug for several days now.  But it has many
layers, and every time I peel off one layer I find another one.

This bug is a recent regression -- the STR from comment #0 only starts
working as of the following comm-central nightly:

thunderbird-2011-21-21-03-00-22-comm-central

The proximate cause of the crashes is that, as of this nightly, the
"hidden window" no longer has a menubar.  But I can't tell which of
the patches in this range caused this to start happening, so I'll need
to hg bisect.
Comment 9 Steven Michaud [:smichaud] (Retired) 2012-02-28 10:12:59 PST
Here's the patch that triggered this bug:

Bug 644169 - Implement Tabs on Top for Thunderbird. ui-r=bwinton,r=sid0,sr=Standard8.
author	Mike Conley <mconley@mozilla.com>
	Tue Dec 20 17:12:32 2011 -0500 (at Tue Dec 20 17:12:32 2011 -0500)
http://hg.mozilla.org/comm-central/rev/3f507b7d7ee6

Mike, do you know how your patch stopped the "hidden window" from having a menubar?
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-02-28 10:21:58 PST
Hey Steven,

Hm.  So we moved the menubar inside a toolbox named "navigation-toolbox" in mailWindowOverlay.xul.

I'm not 100% sure which hidden window you're referring to. Is it defined somewhere in TB's XUL?

-Mike
Comment 11 Steven Michaud [:smichaud] (Retired) 2012-02-28 10:53:24 PST
The "hidden window" is Mac-specific, I believe.  I'm not entirely sure how it works (or what it's for).  But Cocoa widget code assumes its existence, and assumes that it has a menubar.

Here are two definitions for it -- one for Firefox and one for Thunderbird:

./mozilla/browser/base/content/hiddenWindow.xul
./mail/base/content/hiddenWindow.xul
Comment 12 Steven Michaud [:smichaud] (Retired) 2012-02-28 10:54:44 PST
Note that Firefox's "hidden window" still has a menubar (so that the STR from comment #0 doesn't work with current trunk Firefox).
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-02-28 11:02:05 PST
Hm - so it looks like hiddenWindow.xul is hooking into mail-toolbox, which no longer has the menubar:

http://mxr.mozilla.org/comm-central/source/mail/base/content/hiddenWindow.xul#106

It's possible that hiddenWindow.xul wants navigation-toolbox there instead.

Does this resolve the crash?
Comment 14 Steven Michaud [:smichaud] (Retired) 2012-02-28 11:03:40 PST
> It's possible that hiddenWindow.xul wants navigation-toolbox there instead.
>
> Does this resolve the crash?

I'll try this and let you know my results.
Comment 15 Steven Michaud [:smichaud] (Retired) 2012-02-28 12:33:46 PST
Created attachment 601372 [details] [diff] [review]
Proposed fix

(Following up comment #14)

It worked!  This change	restores the hidden window's menubar, and
gets rid of this bug's crashes.

The reason for the crashes is that a static native "application menu"
(sApplicationMenu in mozilla/widget/cocoa/nsMenuBarX.mm) is created
whenever the hidden window's menubar is created, which in some sense
"belongs" to the hidden window's menubar (it shares some resources
with it).  This application menu is "static" in the sense that it's
created only once, but is passed around as needed among	all the
browser windows.  So it needs to live as long as the application
itself (i.e. as long as the hidden window).

If the hidden window doesn't have a menubar, sApplicationMenu instead
gets created when the first browser window's menubar gets created, and
goes out of scope when that window (and its menubar) is destroyed --
causing crashes and other problems.

In my brief tests this patch caused no problems.  I paid particular
attention to the "application menu" -- which is the leftmost menu
after the Apple menu (the "Daily" menu in comm-central builds).  But I
don't know enough about Thunderbird (or about the hidden window	in any
application) to be sure I didn't miss something.

Who should review my patch?  Should I ask you to do it, Mike? 

I've started a tryserver build, which should be available in a few
hours.
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-02-28 12:41:15 PST
Sure, go ahead and r? me.
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-02-28 12:41:26 PST
Thanks for working on this, by the way. :)
Comment 18 Steven Michaud [:smichaud] (Retired) 2012-02-28 12:48:56 PST
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

> Thanks for working on this, by the way. :)

You're quite welcome.  Thank you for figuring out how to give the hidden window back its menubar.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-02-29 06:33:51 PST
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

This looks good to me, and I can confirm that the crash is fixed.
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-02-29 06:35:31 PST
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

We're definitely going to want this for TB 11 and 12.
Comment 21 Mark Banner (:standard8) 2012-02-29 06:51:21 PST
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

Yep, please land it asap.
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-02-29 07:04:43 PST
Committed to:

comm-central as http://hg.mozilla.org/comm-central/rev/82295114c5a6
comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/315ceda5ffe6
comm-beta as http://hg.mozilla.org/releases/comm-beta/rev/3a8256a08f2d

Thanks for your work, Steven!  You rock!
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2012-02-29 07:17:43 PST
(In reply to Steven Michaud from comment #15)
> If the hidden window doesn't have a menubar, sApplicationMenu instead
> gets created when the first browser window's menubar gets created, and
> goes out of scope when that window (and its menubar) is destroyed --
> causing crashes and other problems.

Well, shouldn't we fail in a more obvious manner then? e.g. don't display any menu if the hidden window doesn't have a menu.
Comment 24 Steven Michaud [:smichaud] (Retired) 2012-02-29 08:01:40 PST
(In reply to comment #23)

No, not	really.

All the "hiddenWindow" objects contain comments	indicating it needs a
menubar.  Mike missed this in his patch (because he didn't actually
change any of the hiddenMenu objects).  This caused problems in Cocoa
widget code that led to	the bug being given to someone (me) who could
figure it out from the Cocoa widget side.

This would all still have happened if the symptom had been a missing
menu.  Though I admit it might have saved me a few hours figuring
out the problem.

This is yet another example (and there are hundreds) of an implicit
"API" governing relations between different parts of the tree.  As
long as it isn't "public", it doesn't really need to be formalized.
Though you do need comments explaining it -- which in fact we already
have.
Comment 25 Siddharth Agarwal [:sid0] (inactive) 2012-02-29 10:02:59 PST
(In reply to Steven Michaud from comment #24)
> (In reply to comment #23)
> 
> No, not	really.
> 
> All the "hiddenWindow" objects contain comments	indicating it needs a
> menubar.  Mike missed this in his patch (because he didn't actually
> change any of the hiddenMenu objects).  This caused problems in Cocoa
> widget code that led to	the bug being given to someone (me) who could
> figure it out from the Cocoa widget side.
> 
> This would all still have happened if the symptom had been a missing
> menu.  Though I admit it might have saved me a few hours figuring
> out the problem.

Or, well, some text in the menu bar saying "No menu found, check that your hiddenWindow has a menu". My point is that from the way you describe it, hiddenWindow without a menu is a thoroughly broken state anyway, so any attempts to fix it are only going to make it worse.
Comment 26 Steven Michaud [:smichaud] (Retired) 2012-02-29 10:07:35 PST
> My point is that from the way you describe it, hiddenWindow without
> a menu is a thoroughly broken state anyway, so any attempts to fix
> it are only going to make it worse.

I don't understand.  What do you mean by "any attempts to fix it are
only going to make it worse"?

Cocoa widget code assumes that a hidden window will always exist, and
that it will have a menubar.  I don't see anything wrong with this.

Similar	assumptions are made all over the tree.
Comment 27 Siddharth Agarwal [:sid0] (inactive) 2012-02-29 10:09:24 PST
Ah, I see. Never mind.

Note You need to log in before you can comment on or make changes to this bug.