[Audio] The Usage of nsRefPtr for AudioChannelService is Wrong

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mchen, Assigned: mchen)

Tracking

unspecified
B2G C2 (20nov-10dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

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

Comment 1

7 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee

Comment 2

7 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #689636 - Attachment is obsolete: true
Assignee

Comment 3

7 years ago
Comment on attachment 689648 [details] [diff] [review]
Patch v1

Dear Robert,

Sorry for this mistake. And fix it right away.
Attachment #689648 - Flags: review?(roc)
Assignee

Comment 4

7 years ago
The side effect from Bug 815069 and it should be fixed.
blocking-basecamp: --- → ?
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.
Assignee

Comment 6

7 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #689648 - Attachment is obsolete: true
Attachment #689648 - Flags: review?(roc)
Attachment #689651 - Flags: review?(roc)
Assignee

Comment 7

7 years ago
Thanks for this quickly review and response. Follow the comment on the patch.
Assignee

Comment 8

7 years ago
Attachment #689651 - Attachment is obsolete: true
Attachment #689659 - Flags: review+
Assignee

Comment 9

7 years ago
Dear Robert,

Thanks.

Dear Jonas,

Please help to judge the bb?. Very thanks for your help.
Flags: needinfo?(jonas)
blocking-basecamp: ? → +
Flags: needinfo?(jonas)
Assignee

Updated

7 years ago
Keywords: checkin-needed
Assignee

Updated

7 years ago
Blocks: 819500
Assignee

Updated

7 years ago
Blocks: 819823
Duplicate of this bug: 819864
https://hg.mozilla.org/mozilla-central/rev/2224b34281aa
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 820068

Updated

7 years ago
Duplicate of this bug: 820069
Whiteboard: [status-b2g18:fixed]
Whiteboard: [status-b2g18:fixed]
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.
FYI, please don't use |static nsRefPtr| instances.  They create static constructors / destructors, which make Firefox start up slower.

Instead, use |static StaticRefPtr|.
Assignee

Comment 19

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