Closed Bug 728964 Opened 12 years ago Closed 12 years ago

Crash [@ -[NativeMenuItemTarget menuItemHit:] ] when 'About Thunderbird' is clicked

Categories

(Thunderbird :: Mail Window Front End, defect)

11 Branch
x86
macOS
defect
Not set
critical

Tracking

(thunderbird11+ fixed, thunderbird12 fixed)

RESOLVED FIXED
Thunderbird 13.0
Tracking Status
thunderbird11 + fixed
thunderbird12 --- fixed

People

(Reporter: kyle.kalstabakken, Assigned: smichaud)

References

()

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(2 files)

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.
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
Keywords: crash
Thanks.
Keywords: crashreportid
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: General → Widget: Cocoa
Ever confirmed: true
Keywords: crashreportid
Product: Thunderbird → Core
QA Contact: general → cocoa
Version: 11 → 11 Branch
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?
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).
Assignee: nobody → smichaud
Keywords: reproducible
> 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.
Summary: Crash when 'About Thunderbird' is clicked → Crash [@ -[NativeMenuItemTarget menuItemHit:] ] when 'About Thunderbird' is clicked
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.
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?
Blocks: tb-tabsontop
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
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
Note that Firefox's "hidden window" still has a menubar (so that the STR from comment #0 doesn't work with current trunk Firefox).
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?
> 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.
Attached patch Proposed fixSplinter Review
(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.
Sure, go ahead and r? me.
Thanks for working on this, by the way. :)
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.
Attachment #601372 - Flags: review?(mconley)
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

This looks good to me, and I can confirm that the crash is fixed.
Attachment #601372 - Flags: review?(mconley) → review+
Assignee: smichaud → nobody
Component: Widget: Cocoa → Mail Window Front End
Product: Core → Thunderbird
QA Contact: cocoa → front-end
Target Milestone: --- → Thunderbird 13.0
Version: 11 Branch → 11
Assignee: nobody → smichaud
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

We're definitely going to want this for TB 11 and 12.
Attachment #601372 - Flags: approval-comm-beta?
Attachment #601372 - Flags: approval-comm-aurora?
Comment on attachment 601372 [details] [diff] [review]
Proposed fix

Yep, please land it asap.
Attachment #601372 - Flags: approval-comm-beta?
Attachment #601372 - Flags: approval-comm-beta+
Attachment #601372 - Flags: approval-comm-aurora?
Attachment #601372 - Flags: approval-comm-aurora+
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!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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.
(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.
(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.
> 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.
Ah, I see. Never mind.
Depends on: 738074
Blocks: 843551
You need to log in before you can comment on or make changes to this bug.