Make nsImageDocument/nsPluginDocument participate better in cycle collection

RESOLVED FIXED in mozilla1.9.1b3

Status

()

defect
RESOLVED FIXED
11 years ago
2 months ago

People

(Reporter: peterv, Assigned: smaug)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

11 years ago
Posted patch v1Splinter Review
No description provided.
Attachment #346669 - Flags: superreview?(peterv)
Attachment #346669 - Flags: review?(peterv)
Reporter

Updated

11 years ago
Attachment #346669 - Flags: superreview?(peterv)
Attachment #346669 - Flags: superreview+
Attachment #346669 - Flags: review?(peterv)
Attachment #346669 - Flags: review+
Reporter

Comment 1

11 years ago
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?
Assignee

Comment 2

11 years ago
(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.
Assignee

Comment 3

11 years ago
(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.
Assignee

Updated

11 years ago
Attachment #346669 - Flags: approval1.9.1?
Can I get a summary of risk and reward here?
Assignee

Comment 5

11 years ago
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+
Assignee

Updated

11 years ago
Whiteboard: [needs-1.9.1-landing]
Assignee

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Assignee

Comment 7

11 years ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.