Closed Bug 463424 Opened 16 years ago Closed 16 years ago

Make nsImageDocument/nsPluginDocument participate better in cycle collection

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: peterv, Assigned: smaug)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

Attached patch v1Splinter Review
      No description provided.
Attachment #346669 - Flags: superreview?(peterv)
Attachment #346669 - Flags: review?(peterv)
Attachment #346669 - Flags: superreview?(peterv)
Attachment #346669 - Flags: superreview+
Attachment #346669 - Flags: review?(peterv)
Attachment #346669 - Flags: review+
Comment on attachment 346669 [details] [diff] [review]
v1

>diff --git a/content/html/document/src/nsImageDocument.cpp b/content/html/document/src/nsImageDocument.cpp

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsImageDocument, nsMediaDocument)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mImageContent)

Hmm, do we need to add Traverse/Unlink to nsImageLoadingContent too, in particular for its observers?

>diff --git a/content/html/document/src/nsPluginDocument.cpp b/content/html/document/src/nsPluginDocument.cpp

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsPluginDocument, nsMediaDocument)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mPluginContent)

Only risk here looks like nsPluginStreamListener::OnStartRequest uses this unconditionally. No idea if we can unlink before that's called?
(In reply to comment #1) 
> Hmm, do we need to add Traverse/Unlink to nsImageLoadingContent too, in
> particular for its observers?
Maybe. That isn't needed to pass mochitest without leaks.
"Typically we will have only one observer (our frame in the screen
* prescontext)"
The frame will be destroyed early enough. But if there are other kinds of observers, then not sure.
(In reply to comment #1)
> Only risk here looks like nsPluginStreamListener::OnStartRequest uses this
> unconditionally.
Actually it doesn't.
shell->GetPrimaryFrameFor(embed); returns nsnull if embed is nsnull.
Attachment #346669 - Flags: approval1.9.1?
Can I get a summary of risk and reward here?
This fixes possible leaks (I think I could even write a leak testcase).
Can't see any real risks.
Comment on attachment 346669 [details] [diff] [review]
v1

a191=beltzner
Attachment #346669 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs-1.9.1-landing]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Fixed on trunk and 1.9.1
Whiteboard: [needs-1.9.1-landing]
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: