Closed
Bug 323807
Opened 19 years ago
Closed 19 years ago
event listener wrapper preservation can leave dangling nsIDOMGCParticipant in preserved wrapper table [@ ClassifyWrapper]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: crash, fixed1.8.1, topcrash, Whiteboard: [patch])
Crash Data
Attachments
(1 file)
18.10 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
I think I only half-addressed bug 241518 comment 39: I solved the problem of dangling wrappers, but not the problem of dangling nsIDOMGCParticipants. At least I don't think I did. This would cause a crash in ClassifyWrapper if the nsIDOMGCParticipant has been destroyed. For the other cases (where the wrapper wraps the participant), we use the ReleaseWrapper call in nsEventReceiverSH::Finalize. This is probably the cause of the crash in bug 323371 -- ClassifyWrapper shows up on the topcrash list.
Assignee | ||
Updated•19 years ago
|
Keywords: topcrash
Summary: event listener wrapper preservation can leave dangling nsIDOMGCParticipant in preserved wrapper table → event listener wrapper preservation can leave dangling nsIDOMGCParticipant in preserved wrapper table [@ ClassifyWrapper]
Assignee | ||
Comment 1•19 years ago
|
||
Hrm. Now that I think about it some more, this would only actually crash if we leak the event listener manager, since otherwise we will be guaranteed to have made the appropriate ReleaseWrapper call in nsMarkedJSFunctionHolder_base::~nsMarkedJSFunctionHolder_base.
Assignee | ||
Comment 2•19 years ago
|
||
It's also worth noting that the nsEventReceieverSH::Finalize (or nsDOMGCParticipantSH::Finalize after bug 323534) is the wrong place to fix this, since if the node's wrapper isn't preserved (but only the event listener), then the node's wrapper can be finalized while the node is still reachable.
Updated•19 years ago
|
Severity: normal → critical
Assignee | ||
Comment 3•19 years ago
|
||
This is still compiling; not tested yet, and may not compile. I came to the conclusion that there's no general way to fix this, so I just need to document that GC participant destructors (or, more generally, the caller to PreserveWrapper) has to ensure that ReleaseWrapper happens before the participant goes away. I did that by removing all event listeners before letting go of the event listener manager. I also found a bunch of places that should have been calling SetListenerTarget(nsnull), but weren't, so I combined the two into Disconnect.
Assignee | ||
Updated•19 years ago
|
Whiteboard: [patch]
Assignee | ||
Updated•19 years ago
|
Attachment #208801 -
Attachment description: patch (untested) → patch
Attachment #208801 -
Flags: superreview?(jst)
Attachment #208801 -
Flags: review?(mrbkap)
Comment 4•19 years ago
|
||
Comment on attachment 208801 [details] [diff] [review] patch Seems reasonable to me.
Attachment #208801 -
Flags: review?(mrbkap) → review+
Comment 5•19 years ago
|
||
Comment on attachment 208801 [details] [diff] [review] patch sr=jst
Attachment #208801 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 6•19 years ago
|
||
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•19 years ago
|
||
FWIW, I had to land a very simple bustage fix to nsEventListenerManager.cpp (add "return" before the last line in Disconnect).
Assignee | ||
Comment 8•19 years ago
|
||
Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.
Keywords: fixed1.8.1
Updated•13 years ago
|
Crash Signature: [@ ClassifyWrapper]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•