Closed
Bug 200058
Opened 22 years ago
Closed 22 years ago
standalone images and plugins leak (the whole document leaks)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: peterl-bugs)
Details
(Keywords: memory-leak, regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #119076 -
Flags: superreview?(jst)
Attachment #119076 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 2•22 years ago
|
||
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...
Updated•22 years ago
|
Attachment #119076 -
Attachment is obsolete: true
Attachment #119076 -
Flags: superreview?(jst)
Attachment #119076 -
Flags: review?(bzbarsky)
Comment 3•22 years ago
|
||
okay, here I clear out mStreamListener in SetScriptGlobalObject(0).
Updated•22 years ago
|
Attachment #119113 -
Flags: superreview?(bzbarsky)
Attachment #119113 -
Flags: review?(jkeiser)
Comment 4•22 years ago
|
||
need to have |mStreamListener = nsnull| outside the |if (mImageRequest)|
Attachment #119113 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #119113 -
Flags: superreview?(bzbarsky)
Attachment #119113 -
Flags: review?(jkeiser)
Updated•22 years ago
|
Attachment #119116 -
Flags: superreview?(bzbarsky)
Attachment #119116 -
Flags: review?(jkeiser)
Comment 5•22 years ago
|
||
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+
Reporter | ||
Comment 6•22 years ago
|
||
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+
Comment 7•22 years ago
|
||
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: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•