200058 - standalone images and plugins leak (the whole document leaks)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Peter Lubczynski]

Attachment: patch v.1 (clears mDocument in onStop)

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.

Comment 2

[Boris Zbarsky [: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...

Comment 3

[Peter Lubczynski]

Attachment: patch v.2

okay, here I clear out mStreamListener in SetScriptGlobalObject(0).

Comment 4

[Peter Lubczynski]

Attachment: patch v2.1

need to have |mStreamListener = nsnull| outside the |if (mImageRequest)|

Comment 5

[Johnny Stenback (:jst)]

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).

Comment 6

[Boris Zbarsky [:bzbarsky]]

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...

Comment 7

[Peter Lubczynski]

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!