Closed
Bug 326959
Opened 18 years ago
Closed 18 years ago
menupopup.enableRollup(true) causes crash [@ nsPopupSetFrame::Destroy]
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: jruderman, Assigned: asqueella)
References
Details
(Keywords: crash, testcase)
Crash Data
Attachments
(3 files, 2 obsolete files)
|
486 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
|
11.90 KB,
text/plain
|
Details | |
|
30.38 KB,
patch
|
Details | Diff | Splinter Review |
|
Reporter | |
Comment 1•18 years ago
|
||
|
|
Reporter | |
Comment 2•18 years ago
|
||
|
|
Reporter | |
Updated•18 years ago
|
Summary: menupopup.enableRollup(true) causes crash → menupopup.enableRollup(true) causes crash [@ nsPopupSetFrame::Destroy]
|
Assignee | |
Comment 3•18 years ago
|
||
The crash happens because EnableRollup is written to be called only when the the popup in question is already open. (Otherwise, it creates the nsMenuDismissalListener, but doesn't set its CurrentMenuParent, meaning the created object is in pretty broken state.) I changed enableRollup implementation to be a bit less fragile. It still only has effect when the popup it's called on is open, but it now allows calling EnableRollup multiple times (also when the popup is not open), without getting into broken state. (ignore the chunk in nsMenuFrame::SetInitialChildList, it is for bug 330456) (I'm still new to mozilla's C++ and this code in particular, so please review with care :) By the way, this code assumed (and assumes) that all the open popups are in the same "chain". Is it correct assumption and should bugs be filed when this isn't the case?
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Attachment #215813 -
Flags: superreview?(roc)
Attachment #215813 -
Flags: review?(neil)
|
||
Comment 4•18 years ago
|
||
Comment on attachment 215813 [details] [diff] [review] patch >-nsMenuBarFrame :: KillPendingTimers ( ) >+nsMenuBarFrame::KillPendingTimers ( ) I'm not a fan of random cleanup. I'm not sure I really understand dismissal listeners. I don't suppose you can explain why we need to be able to have a listener without a menu parent?
|
|
Assignee | |
Comment 5•18 years ago
|
||
> I'm not sure I really understand dismissal listeners. I don't suppose you can
> explain why we need to be able to have a listener without a menu parent?
We don't need that. The null-check in nsPopupSetFrame::Destroy can be removed, not sure why I added it :-/
|
|
Assignee | |
Comment 6•18 years ago
|
||
Without the null-check and with fewer jst-review nits.
Attachment #215813 -
Attachment is obsolete: true
Attachment #215919 -
Flags: superreview?(roc)
Attachment #215919 -
Flags: review?(neil)
Attachment #215813 -
Flags: superreview?(roc)
Attachment #215813 -
Flags: review?(neil)
Attachment #215919 -
Flags: superreview?(roc) → superreview+
|
|
||
Comment 7•18 years ago
|
||
Comment on attachment 215919 [details] [diff] [review] patch v2 Seems okay then.
Attachment #215919 -
Flags: review?(neil) → review+
|
|
Assignee | |
Updated•18 years ago
|
OS: MacOS X → All
Hardware: Macintosh → All
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
|
|
Assignee | |
Comment 8•18 years ago
|
||
Without the whitespace change in KillPendingTimers and the change for bug 330456.
Attachment #215919 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 9•18 years ago
|
||
Checked in by timeless.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Verified FIXED using build 2006-03-24-04 of SeaMonkey trunk on Windows XP; no longer crashing.
Status: RESOLVED → VERIFIED
|
||
Comment 11•18 years ago
|
||
*** Bug 305423 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
|
||
Updated•13 years ago
|
Crash Signature: [@ nsPopupSetFrame::Destroy]
You need to log in
before you can comment on or make changes to this bug.
Description
•