849519 - Undefined volume causes sound to crash

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Maxence D.]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130307023931

Steps to reproduce:

var audio = new Audio;
audio.src = "http://opengameart.org/sites/default/files/Hellin_0.ogg";
audio.addEventListener('canplaythrough', function() {
    this.volume = undefined;
    this.play();
});


Actual results:

Browser's sound doesn't play anymore after running this code, in any tab, any website

Comment 1

[Scoobidiver (away)]

Please provide a testcase as an attachment.

Comment 2

[Matthew Gregan [:kinetik]]

With a video playing in another tab, running the linked JSFiddle doesn't give me the behaviour described in the initial comment, but it does hang the browser (including the playing video) for around 10 seconds... after which, it recovers and the video resumes playback with working audio.

Comment 3

[Maxence D.]

I'm surprised you can't reproduce this one. Watch a Youtube video with the sound on in an other tab, then just open the JSFiddle, wait a few seconds that the ogg sound can load (add a console.log() if you want) and you'll get no sound anymore in the browser (it makes my computer sound mute too !). The sound comes back after about a minute. There's no problem with this test-case with chrome, so I don't think it's because of my computer.

Comment 4

[Matthias Versen [:Matti]]

I could reproduce this with  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 SeaMonkey/2.19a1 

The sound in a background flash based video disappeared and it didn't come back.

Comment 5

[Matthias Versen [:Matti]]

It seems that my Realtek audio driver crashed and refused to play any sound (even in VLC) and i had to reboot. Stupid driver bug....

Comment 6

[Matthew Gregan [:kinetik]]

So the difference between what I see and what Matti and maxigamer59 see is probably down to driver differences.

The core issue is that we shouldn't be trying to use "undefined" in any volume calculations, so this should never reach the audio driver.

In a debug build, we hit:
  #3  mozilla::BufferedAudioStream::SetVolume (this, aVolume=nan) at content/media/AudioStream.cpp:850
  850	  NS_ABORT_IF_FALSE(aVolume >= 0.0 && aVolume <= 1.0, "Invalid volume");

The problem is:
  NS_IMETHODIMP nsHTMLMediaElement::SetVolume(double aVolume)
  {
    if (aVolume < 0.0 || aVolume > 1.0)

This test allows nan to pass through.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Invert aVolume validity test in nsHTMLMediaElement::SetVolume to catch NaN.

Comment 8

[Matthew Gregan [:kinetik]]

s/Invert/Extend/

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 723277 [details] [diff] [review]
patch v0

Let's try that again, post brain waking up.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: patch v1

With correct logic, and a mochitest.

Comment 11

[Maxence D.]

(In reply to Matthew Gregan [:kinetik] from comment #6)
> So the difference between what I see and what Matti and maxigamer59 see is
> probably down to driver differences.
> 
> The core issue is that we shouldn't be trying to use "undefined" in any
> volume calculations, so this should never reach the audio driver.
> 
> In a debug build, we hit:
>   #3  mozilla::BufferedAudioStream::SetVolume (this, aVolume=nan) at
> content/media/AudioStream.cpp:850
>   850	  NS_ABORT_IF_FALSE(aVolume >= 0.0 && aVolume <= 1.0, "Invalid
> volume");
> 
> The problem is:
>   NS_IMETHODIMP nsHTMLMediaElement::SetVolume(double aVolume)
>   {
>     if (aVolume < 0.0 || aVolume > 1.0)
> 
> This test allows nan to pass through.

Well, it makes no sense to use undefined for a volume. I've accidentally discovered this bug by passing a wrong parameter to a Coffeescript code.

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Maxence D. from comment #11)
> Well, it makes no sense to use undefined for a volume. I've accidentally
> discovered this bug by passing a wrong parameter to a Coffeescript code.

Right, and the spec requires that setting volume to a value outside of [0, 1] raises an exception, but our implementation had a bug in that validation logic that allowed NaN to pass through.  With the patch attached to this bug, it will now raise an IndexSizeError exception.

Thanks for reporting this!

Comment 13

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cad83c03950

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cad83c03950