Closed Bug 864518 Opened 7 years ago Closed 7 years ago

HTMLMediaElement::mAudioChannelAgent not declared to CC

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: rillian, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

In his review of bug 833385, bz pointed out I needed to add my new member pointers to the GC class descriptor with NS_IMPL_CYCLE_COLLECTION_TRAVERSE().

I noticed that bug 815069 added mAudioChannelAgent to HTMLMediaElement without updating the cycle collection. I thought I'd check if there was some reason the channel agent can't outlive the parent or if this is an oversight.

If it's safe, perhaps add a comment documenting that?
I don't see how it's safe.
Blocks: 815069
Summary: HTMLMediaElement::mAudioChannelAgent not declared to GC → HTMLMediaElement::mAudioChannelAgent not declared to CC
Attached patch patch (obsolete) — Splinter Review
Is this enough?
Attachment #740670 - Flags: review?(bzbarsky)
Assignee: mchen → amarchesini
Hi all,

As I knew that mAudioChannelAgent is only counted by HTMLMediaElement with nsCOMPtr, so it will be destroyed after HTMLMediaElement's life cycle is end. And this AudioChannelAgent class will not be created from content actively. 

So is it necceary to join garbage collection? I am not familar with GC, so please correct me then learn by this case.

Thanks.
Comment on attachment 740670 [details] [diff] [review]
patch

Should be, yes.  r=me
Attachment #740670 - Flags: review?(bzbarsky) → review+
Comment on attachment 740670 [details] [diff] [review]
patch

Actually, I was wrong about it being enough.  AudioChannelAgent needs to participate in CC as well, and CC mCallback, for this to make any sense.

Marco, the problem is that the HTMLMediaElement holds a reference to the AudioChannelAgent.  And the AudioChannelAgent holds a reference to its mCallback.  But the mCallback is the HTMLMediaElement itself.  So they form a reference cycle, and the HTMLMediaElement cannot be destroyed until the AudioChannelAgent is destroyed, and vice versa.

We have a cycle collector to handle such situations; this has nothing to do with garbage collection.  See https://developer.mozilla.org/en-US/docs/Interfacing_with_the_XPCOM_cycle_collector

In this case, either we need to make sure that the cycle is always broken manually (by guaranteeing that something sets one of the two nsCOMPtrs to null at some point when we want the objects to go away) or we need to make sure that both classes implement cycle collection and notify the cycle collector about the references between them.  Which is the right approach depends on what the exact ownership model of this code is and on whether the AudioChannelAgent can ever be exposed to script; if it can, then they MUST both implement cycle collection, no questions asked.
Attachment #740670 - Flags: review+ → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #740670 - Attachment is obsolete: true
Attachment #740781 - Flags: review?(bzbarsky)
Comment on attachment 740781 [details] [diff] [review]
patch

>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(AudioChannelAgent)
>+NS_IMPL_CYCLE_COLLECTION_TRACE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AudioChannelAgent)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCallback)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AudioChannelAgent)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mCallback)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END

Replace all that with  NS_IMPL_CYCLE_COLLECTION_1(AudioChannelAgent, mCallback)

>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AudioChannelAgent)

Just NS_DECL_CYCLE_COLLECTION_CLASS.

r=me with that
Attachment #740781 - Flags: review?(bzbarsky) → review+
Attached patch patchSplinter Review
Attachment #740781 - Attachment is obsolete: true
Attachment #740842 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34d00e20ff2d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 740670 [details] [diff] [review]
> patch
> 
> Actually, I was wrong about it being enough.  AudioChannelAgent needs to
> participate in CC as well, and CC mCallback, for this to make any sense.
> 
> Marco, the problem is that the HTMLMediaElement holds a reference to the
> AudioChannelAgent.  And the AudioChannelAgent holds a reference to its
> mCallback.  But the mCallback is the HTMLMediaElement itself.  So they form
> a reference cycle, and the HTMLMediaElement cannot be destroyed until the
> AudioChannelAgent is destroyed, and vice versa.
> 

Hi Boris,

Thanks for your kindly explanation in first.

About the reference to each other, AudioChannelAgent just hold a weak reference to HTMLMediaElement only but strong refrence.  So I think there is no issue on reference to each other in this case. Is this right?

Thanks for Andrea, Ralph and Boris help again.
> AudioChannelAgent just hold a weak reference to HTMLMediaElement

Ah, because of the InitWithWeakCallback bit?

And script can never get its hands on the AudioChannelAgent?

(On a side note, what breaks the cycle between FMRadio and its AudioChannelAgent?  That seems to be the consumer of Init(), right?)
> And script can never get its hands on the AudioChannelAgent?

Script can do that. I do that here: Bug 862899
OK, if script can get its hands on the AudioChannelAgent, then we can get into a situation where the media element holds a ref to the AudioChannelAgent, the AudioChannelAgent holds a ref to its JS reflection, and the JS reflection points to the HTMLMediaElement's JS reflection.

Or at least we can get there once AudioChannelAgent is ever wrappercached.

But certainly the patch in bug 862899 needs the CC bits added here to not leak, unless we're guaranteed that disable() is always called.

So the point is, not cycle-collecting things means that leaking suddenly gets very very easy.
Hi Boris & Andrea,

> Script can do that. I do that here: Bug 862899

This is true but not for the case of HTMLMediaElement. The AudioChannelAgent held by HTMLMediaElement is created by C++ code not JS. So I agree that AudioChannelAgent should be added into GC in case of FMRadio but HTMLMediaElement.

So in case of HTMLMediaElement, it needs to add itself into GC because it is possible to be created from JS scope. Once GC delete HTMLMediaElement, AudioChannelAgent will be deleted too. So may I say AudioChannelAgent in HTMLMediaElement is no necessary to join GC? 

I just want to clarify the criteria of member data joining GC so I will not miss it next time. Thanks for all your class here.
The general rule of thumb I would use is that if you hold a strong pointer to something you should cycle-collect it.

Anything else depends on making nonlocal inferences about the object graph which can easily become false as other parts of the object graph change.  For example, in this case you have to prove that no outgoing references from the AudioChannelAgent can ever get back to the HTMLMediaElement and that this will remain the case as AudioChannelAgent is changed in the future...
Hi Boris,

So may I say that adding AudioChannelAgent in HTMLMediaElement into GC is for safe now? Because it just own a weak reference to HTMLMediaElement and not reference by JS. But in order to prevent any change of it in the future which will cause leaking. We still decide to add it.

If this is right, I think I get more knowledge about GC now. Thanks Boris.
> So may I say that adding AudioChannelAgent in HTMLMediaElement into GC is for safe now? 

It's possible, yes, given the above discussion.

Please note that this is the _cycle_ collector, not the SpiderMonkey _garbage_ collector we're talking about here; the difference is important...
You need to log in before you can comment on or make changes to this bug.