Closed Bug 849519 Opened 7 years ago Closed 7 years ago

Undefined volume causes sound to crash

Categories

(Core :: Audio/Video, defect)

19 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: maxigamer59, Assigned: kinetik)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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
Severity: normal → minor
Please provide a testcase as an attachment.
Severity: minor → normal
Component: Untriaged → Video/Audio
Keywords: testcase-wanted
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
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....
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attached patch patch v0 (obsolete) — Splinter Review
Invert aVolume validity test in nsHTMLMediaElement::SetVolume to catch NaN.
Attachment #723277 - Flags: review?(chris.double)
s/Invert/Extend/
Comment on attachment 723277 [details] [diff] [review]
patch v0

Let's try that again, post brain waking up.
Attachment #723277 - Attachment is obsolete: true
Attachment #723277 - Flags: review?(chris.double)
Attached patch patch v1Splinter Review
With correct logic, and a mochitest.
Attachment #723281 - Flags: review?(chris.double)
(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.
Attachment #723281 - Flags: review?(chris.double) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/9cad83c03950
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.