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)
Tracking
(thunderbird11+ fixed, thunderbird12 fixed)
RESOLVED
FIXED
Thunderbird 13.0
People
(Reporter: kyle.kalstabakken, Assigned: smichaud)
References
()
Details
(Keywords: crash, reproducible)
Crash Data
Attachments
(2 files)
37.05 KB,
text/plain
|
Details | |
694 bytes,
patch
|
mconley
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•12 years ago
|
||
bp-ff929447-1951-47b7-8b9c-08e8d2120220
Updated•12 years ago
|
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
Assignee | ||
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Keywords: reproducible
Assignee | ||
Comment 6•12 years ago
|
||
> 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.
Assignee | ||
Updated•12 years ago
|
Summary: Crash when 'About Thunderbird' is clicked → Crash [@ -[NativeMenuItemTarget menuItemHit:] ] when 'About Thunderbird' is clicked
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
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
Comment 10•12 years ago
|
||
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
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 14•12 years ago
|
||
> 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.
Assignee | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
Sure, go ahead and r? me.
Comment 17•12 years ago
|
||
Thanks for working on this, by the way. :)
Assignee | ||
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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+
Updated•12 years ago
|
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
Updated•12 years ago
|
tracking-thunderbird11:
--- → ?
Updated•12 years ago
|
Assignee: nobody → smichaud
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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+
Comment 22•12 years ago
|
||
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
status-thunderbird11:
--- → fixed
status-thunderbird12:
--- → fixed
Resolution: --- → FIXED
Updated•12 years ago
|
Comment 23•12 years ago
|
||
(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.
Assignee | ||
Comment 24•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 26•12 years ago
|
||
> 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•12 years ago
|
||
Ah, I see. Never mind.
You need to log in
before you can comment on or make changes to this bug.
Description
•