Closed Bug 323534 Opened 18 years ago Closed 18 years ago

createTreeWalker can cause leaks due to cycles created by closures

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: fixed1.8.1, memory-leak, Whiteboard: [patch])

Attachments

(1 file, 1 obsolete file)

In bug 285065 I fixed a bug where createTreeWalker led to a cycle by fixing the caller.  Jesse pointed out that we should fix the general problem.  This could be fixed using the same approach as bug 241518.
Attached patch patch (obsolete) — Splinter Review
Compiles; not yet tested.
I need to check if anything has treewalker member variables, though.
(FWIW, this approach in general is sort of bad for the C++ API -- it makes it hard for somebody to use TreeWalker from C++ with a NodeFilter implementation written in JS.)
Attached patch patchSplinter Review
Oops, forgot to change inheritance of nsEventReceiverSH.

Still not tested.
Attachment #208567 - Attachment is obsolete: true
I confirmed that this patch also fixes bug 285065.
Attachment #208569 - Flags: superreview?(jst)
Attachment #208569 - Flags: review?(mrbkap)
Comment on attachment 208569 [details] [diff] [review]
patch

>+  // XXX nsEventReceiverSH::Finalize: clear event handlers in mListener...

Uber-nit: Line this up with the leftmost column?
r=mrbkap with that.
Attachment #208569 - Flags: review?(mrbkap) → review+
Comment on attachment 208569 [details] [diff] [review]
patch

sr=jst
Attachment #208569 - Flags: superreview?(jst) → superreview+
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
The addEventListener dependency (bug 241518) isn't going to land on the 1.8.0 branch so this can't either.
Flags: blocking1.8.0.3+ → blocking1.8.0.3-
Flags: blocking1.8.1?
Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.
Keywords: fixed1.8.1
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.