Closed Bug 200058 Opened 19 years ago Closed 19 years ago

standalone images and plugins leak (the whole document leaks)

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: peterl-bugs)

Details

(Keywords: memory-leak, regression)

Attachments

(1 file, 2 obsolete files)

This is a regression from bug 90256.  The stream listener holds a strong ref to
the document (via nsRefPtr) and the document holds a strong ref to the listener
(also via nsRefPtr).  As a result the whole shebang (listener, document, all
contents of document, etc) leaks.

For nsImageDocument the fix is trivial -- just don't hold on to the listener at all.

For nsPluginDocument, things are a little more complicated, since it uses the
listener... Perhaps the document should drop its ref to the listener somewhere?
 e.g. in OnStopRequest?  Or perhaps one of the refs can be made weak?

Setting critical, since this is a pretty severe memory leak... Further, this
regresses a similar leak which was recently fixed for nsImageDocument.
Here's a patch to clear and remove the reference to mDocument in
nsMediaDocumentListener::OnStopRequest once we are done with it. This prevents
the circular reference and the leak.
Attachment #119076 - Flags: superreview?(jst)
Attachment #119076 - Flags: review?(bzbarsky)
Comment on attachment 119076 [details] [diff] [review]
patch v.1 (clears mDocument in onStop)

Hmm.... to be safe, could you also drop the document's pointer to the listener
when the script global object is set to null?  That would guard against broken
channel impls that fail to call OnStopRequest...
Attachment #119076 - Attachment is obsolete: true
Attachment #119076 - Flags: superreview?(jst)
Attachment #119076 - Flags: review?(bzbarsky)
Attached patch patch v.2 (obsolete) — Splinter Review
okay, here I clear out mStreamListener in SetScriptGlobalObject(0).
Attachment #119113 - Flags: superreview?(bzbarsky)
Attachment #119113 - Flags: review?(jkeiser)
Attached patch patch v2.1Splinter Review
need to have |mStreamListener = nsnull| outside the |if (mImageRequest)|
Attachment #119113 - Attachment is obsolete: true
Attachment #119113 - Flags: superreview?(bzbarsky)
Attachment #119113 - Flags: review?(jkeiser)
Attachment #119116 - Flags: superreview?(bzbarsky)
Attachment #119116 - Flags: review?(jkeiser)
Comment on attachment 119116 [details] [diff] [review]
patch v2.1

sr=jst (bz, feel free to sr too if you want to have one last look).
Attachment #119116 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 119116 [details] [diff] [review]
patch v2.1

r=me, though I still wonder why the image document is holding on to the
listener at all...
Attachment #119116 - Flags: review?(jkeiser) → review+
good catch bz! The image document doesn't really need a pointer to the stream
listener.  I made that change and checked in the fix, thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.