Ralph Giles (:rillian) Reporter
	Description • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 11 years ago

I don't see how it's safe.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 11 years ago

Is this enough?

	Marco Chen [:mchen]
	Comment 3 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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

Should be, yes.  r=me

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

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

	Ralph Giles (:rillian) Reporter
	Comment 9 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/34d00e20ff2d

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/34d00e20ff2d

	Marco Chen [:mchen]
	Comment 11 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 11 years ago

> And script can never get its hands on the AudioChannelAgent?

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

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

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.

	Marco Chen [:mchen]
	Comment 15 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

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

	Marco Chen [:mchen]
	Comment 17 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 11 years ago

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