Closed Bug 1004306 Opened 6 years ago Closed 5 years ago

AudioChannel implementation can cause multiple sounds without any possibility to stop the sound

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: soeren.hentzschel, Assigned: baku)

Details

Attachments

(1 file, 1 obsolete file)

STR:

1. set media.useAudioChannelService to 'true'
2. set media.defaultAudioChannel to 'content'
3. start playing a video on youtube
4. click on another video in the youtube sidebar before the current video is over

Expected:

The first video should stop and only the second video should play sound.
Actual:

You can hear sound from both videos and there is no possibility to stop the sound of the first video, except you restart the browser. It should not be possible to get Firefox into a state in which the user has no longer the control over the browser.
It seems a problem with Cycle Collection or HTMLMediaElements.
I say this because opening about:memory and forcing CC and GC, the first audio player stops.
Assignee: nobody → amarchesini
Attached patch audio.patch (obsolete) — Splinter Review
So, the issue here is that AudioChannelService keeps alive the MediaElements.
With this patch we removes all the agents when the window is destroyed.
Attachment #8505570 - Flags: review?(mchen)
Comment on attachment 8505570 [details] [diff] [review]
audio.patch

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

Hi Randy, 

Could you help to check speaker part? thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +754,5 @@
> +                                               nsAutoPtr<AudioChannelAgentData>& aData,
> +                                               void* aPtr)
> +{
> +  uint64_t* innerID = static_cast<uint64_t*>(aPtr);
> +  MOZ_ASSERT(*innerID);

Do you want to check whether innerID is 0 or not? 
Or this point is null or not?

@@ +766,5 @@
> +  MOZ_ASSERT(service);
> +
> +  service->UnregisterAudioChannelAgent(aAgent);
> +  service->UnregisterType(aData->mChannel, aData->mElementHidden,
> +                          CONTENT_PROCESS_ID_MAIN, aData->mWithVideo);

1. ::UnregisterType() will be performed inside ::UnregisterAudioChannelAgent() already.

2. Inside ::UnregisterAudioChannelAgent(), this aAgent will be ::RemoveAndForget() from hashtable then the return value - PL_DHASH_REMOVE will do the rawRemove again. Is this the right way to do?
Attachment #8505570 - Flags: review?(mchen) → review?(rlin)
> 1. ::UnregisterType() will be performed inside
> ::UnregisterAudioChannelAgent() already.

Actually you are right and this was not included in my latest version of the patch. I want to do just UnregisterType() and no tUnregisterUserAgent().
Attached patch audio.patchSplinter Review
Attachment #8505570 - Attachment is obsolete: true
Attachment #8505570 - Flags: review?(rlin)
Attachment #8506084 - Flags: review?(rlin)
Attachment #8506084 - Flags: review?(mchen)
Attachment #8506084 - Flags: review?(rlin) → review+
Comment on attachment 8506084 [details] [diff] [review]
audio.patch

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

r+ after addressing the only one questions.

::: dom/audiochannel/AudioChannelService.cpp
@@ +754,5 @@
> +                                               nsAutoPtr<AudioChannelAgentData>& aData,
> +                                               void* aPtr)
> +{
> +  uint64_t* innerID = static_cast<uint64_t*>(aPtr);
> +  MOZ_ASSERT(*innerID);

Does this check for null pointer?
Or does it check for whether innerID is 0?
Attachment #8506084 - Flags: review?(mchen) → review+
> Does this check for null pointer?
> Or does it check for whether innerID is 0?

The second one. but I can add MOZ_ASSERT for the first condition as well.
https://hg.mozilla.org/mozilla-central/rev/5bbe8b64b4b7
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I tested with current Nightly and the issue described in comment 0 seems to be fixed. But sometimes the sound is still playing after closing the tab (not always reproducible). I am not sure: is it the same bug ("can cause multiple sounds without any possibility to stop the sound") or should I open a new bug report?
Flags: needinfo?(amarchesini)
Yes, please. file a bug and put me in CC. Thanks.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.