If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

menupopup.enableRollup(true) causes crash [@ nsPopupSetFrame::Destroy]

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
--
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: Nickolay_Ponomarev)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla1.9alpha1
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
 
(Reporter)

Comment 1

12 years ago
Created attachment 211660 [details]
testcase
(Reporter)

Comment 2

12 years ago
Created attachment 211661 [details]
stack trace (mac debug)
(Reporter)

Updated

12 years ago
Summary: menupopup.enableRollup(true) causes crash → menupopup.enableRollup(true) causes crash [@ nsPopupSetFrame::Destroy]
(Assignee)

Comment 3

12 years ago
Created attachment 215813 [details] [diff] [review]
patch

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

12 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

12 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

12 years ago
Created attachment 215919 [details] [diff] [review]
patch v2

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

12 years ago
Comment on attachment 215919 [details] [diff] [review]
patch v2

Seems okay then.
Attachment #215919 - Flags: review?(neil) → review+
(Assignee)

Updated

12 years ago
OS: MacOS X → All
Hardware: Macintosh → All
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 8

12 years ago
Created attachment 216053 [details] [diff] [review]
patch for checkin

Without the whitespace change in KillPendingTimers and the change for bug 330456.
Attachment #215919 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
Checked in by timeless.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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

Updated

11 years ago
Depends on: 354300

Comment 11

11 years ago
*** Bug 305423 has been marked as a duplicate of this bug. ***

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Crash Signature: [@ nsPopupSetFrame::Destroy]
You need to log in before you can comment on or make changes to this bug.