819275 - [Audio] The Usage of nsRefPtr for AudioChannelService is Wrong

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

AudioChannelAgent tried to use a global nsRefPtr called Foo for AudioChannelService. In each constructors of AudioChannelAgent, the global Foo get AudioChannelService once. And global Foo will be assigned nullptr at each deconstructor. So once 10 instances of AudioChannelAgent are created (actually Foo's ref is only 2, one from AudioChannelService and one from global Foo), the second leave one will clear global Foo. Then following instances will be died while using it.

Comment 1

[Marco Chen [:mchen]]

Attachment: Patch v1

Comment 2

[Marco Chen [:mchen]]

Attachment: Patch v1

Comment 3

[Marco Chen [:mchen]]

Comment on attachment 689648 [details] [diff] [review]
Patch v1

Dear Robert,

Sorry for this mistake. And fix it right away.

Comment 4

[Marco Chen [:mchen]]

The side effect from Bug 815069 and it should be fixed.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 689648 [details] [diff] [review]
Patch v1

Review of attachment 689648 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +63,5 @@
>  
>  /* boolean startPlaying (); */
>  NS_IMETHODIMP AudioChannelAgent::StartPlaying(bool *_retval)
>  {
> +  nsRefPtr<AudioChannelService> service = AudioChannelService::GetAudioChannelService();

You actually don't need to use refptrs here. We can assume/ensure that the service does not go away while an AudioChannelAgent method is running. GetAudioChannelService can return a raw AudioChannelService* pointer and we can use that type for our local variables.

Comment 6

[Marco Chen [:mchen]]

Attachment: Patch v2

Comment 7

[Marco Chen [:mchen]]

Thanks for this quickly review and response. Follow the comment on the patch.

Comment 8

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

Comment 9

[Marco Chen [:mchen]]

Dear Robert,

Thanks.

Dear Jonas,

Please help to judge the bb?. Very thanks for your help.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2224b34281aa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2224b34281aa

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee3892a10709
https://hg.mozilla.org/releases/mozilla-beta/rev/f24f9052578a

Comment 17

[Dominic Kuo [:dkuo]]

Hi Marco,

I am guessing this issue might be related to Bug 821222.

In Bug 821222, the way to reproduce it will cause the audio element set to a new source many times, can you please see if this patch should solve Bug 821222 or it's another issue? thanks.

Comment 18

[Justin Lebar (not reading bugmail)]

FYI, please don't use |static nsRefPtr| instances.  They create static constructors / destructors, which make Firefox start up slower.

Instead, use |static StaticRefPtr|.

Comment 19

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #18)
> FYI, please don't use |static nsRefPtr| instances.  They create static
> constructors / destructors, which make Firefox start up slower.
> 
> Instead, use |static StaticRefPtr|.

Hi Justin,

Thanks for your information here.