1066453 - Leaked file descriptor when nsISound.play is given a file type it can't handle

Status: NEW

(Core :: Widget, defect)

Description

[Nicholas Nethercote [inactive]]

My (non-e10s) trunk builds have been flaky lately. Typically what happens is
about 4--6 hours into the session, when I try to use it after not having
touched it for a while, I get one of the following two behaviours:

- A crash.

- Weird drawing artifacts -- lots of things don't render properly, including
  web content and menus, which makes the browser unusable.

It just happened now and I got this output, variants of which I've seen before:

> console.error: 
>   Could not write session state file 
>   Message: Unix error 24 during operation open on file /home/njn/.mozilla/firefox/aijnow0u.default/sessionstore-backups/recovery.js.tmp (Too many open files)
>   
> console.error: 
>   Could not write session state file 
>   Message: Unix error 24 during operation open on file /home/njn/.mozilla/firefox/aijnow0u.default/sessionstore-backups/recovery.js.tmp (Too many open files)
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'document-open' for stock: Error opening file: Too many open files
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'window-close' for stock: Error opening file: Too many open files
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'document-save-as' for stock: Error opening file: Too many open files
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'document-print-preview' for stock: Error opening file: Too many open files
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'document-print' for stock: Error opening file: Too many open files
> 
> (firefox:3087): Gtk-WARNING **: Error loading theme icon 'application-exit' for stock: Error opening file: Too many open files
> console.error: 
>   Could not write session state file 
>   Message: Unix error 24 during operation open on file /home/njn/.mozilla/firefox/aijnow0u.default/sessionstore-backups/recovery.js.tmp (Too many open files)
>   
> console.error: 
>   Could not write session state file 
>   Message: Unix error 24 during operation open on file /home/njn/.mozilla/firefox/aijnow0u.default/sessionstore.js.tmp (Too many open files)

Perhaps we have an fd leak. I didn't manage to look at /proc/<pid>/fd before
the session ended. I'll try to do that next time.

Comment 1

[Nicholas Nethercote [inactive]]

It happened again and /proc/<pid>/fd tells me that it's ChatZilla's fault. I had an mp3 file set to play when the "normal chat" and "stalk match" sound events occurred, and I was up to 999 open file descriptors, and most of them were for the mp3 file.

Comment 2

[Nicholas Nethercote [inactive]]

Also, ironically enough, I was never able to get the mp3 sound files to actually trigger in Chatzilla. Perhaps there's a single underlying problem causing both symptoms -- the files can't play and as a result they stay open, or something.

Comment 3

[Nicholas Nethercote [inactive]]

This is with Chatzilla 0.9.90.1, on Ubuntu 14.04.

Comment 4

[:Gijs (he/him)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> This is with Chatzilla 0.9.90.1, on Ubuntu 14.04.

CZ uses nsISound, so I fear we'll just punt this back to gecko:

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/irc/xul/content/static.js#1484

Presumably, if you run something like this in the browser console / a browser scratchpad, the same happens:

let s = Cc["@mozilla.org/sound;1"].createInstance(Ci.nsISound);
for (let i = 0; i < 1000; i++) {
  s.play(makeURI("file:///path/to/your/favourite/mp3/sound"));
}


?

Comment 5

[Nicholas Nethercote [inactive]]

The exact code ended up being the following, and focus had to be in a chrome tab for it to run correctly:

> Cu.import("resource://services-common/utils.js");
> let s = Components.classes["@mozilla.org/sound;1"].createInstance(Ci.nsISound);
> for (let i = 0; i < 1000; i++) {
>   s.play(CommonUtils.makeURI("file:///home/njn/Downloads/Robot-blip.mp3"));
> }

Yes, this recreated the problem -- no sound was played, but I ended up with all the open file descriptors.

In the title you used the word "unplayable" -- but if I enter file:///home/njn/Downloads/Robot-blip.mp3 into the address bar in a new tab the mp3 plays just fine.

Comment 6

[Nicholas Nethercote [inactive]]

I added some debugging printfs in widget/gtk/nsSound.cpp. nsSound::Play() is running successfully, not bailing on any exit paths, but doesn't produce any sound.

nsSound::Beep() also runs seemingly successfully, but I can't get a beep sound either.

Comment 7

[Nicholas Nethercote [inactive]]

If I run this command:

> canberra-gtk-play -f ~/Downloads/Robot-blip.mp3

I get this output:

> Failed to play sound: File or data corrupt

Can libcanberra play MP3 files?

Comment 8

[Nicholas Nethercote [inactive]]

> Can libcanberra play MP3 files?

It cannot. However, it can play both .ogg and .wav files. I have to also use the -V option to get a sound out of canberra-gtk-play. So that's progress of a sort, but nsSound::Play() still doesn't produce any sound.

Comment 9

[Nicholas Nethercote [inactive]]

kinetik helped me work out what's happening.

- nsSound::Play() is working with a .ogg file. The volume on my monitor was really low so I couldn't hear it. (For some strange reason when playing it via HTML the volume was higher. Not sure why, but not relevant for this bug.)

- nsSound::Play() fails with a .mp3 file. The call to ca_context_play() returns CA_ERROR_CORRUPT -- which matches the "Failed to play sound: File or data corrupt" I get with canberra-gtk-play -- but that return value is ignored.

Comment 10

[Matthew Gregan [:kinetik]]

On successfully opening a Wave, ca_wav_close will close the FILE*.  For Vorbis, ov_clear does the same.  So it looks like the leak arises because ca_sound_file_open forgets to call fclose() on failure.  I filed a bug against libcanberra upstream: https://bugs.freedesktop.org/show_bug.cgi?id=83959

Comment 11

[Matthew Gregan [:kinetik]]

Thinking about what we can do until a fixed libcanberra is available: we can assume the leak has happened if ca_context_play returns CA_ERROR_CORRUPT, but the FILE* has been lost on the stack, so even if we dig the file descriptor out of /proc/self/fds and close it, we'd leak the FILE*.

We could use NS_SniffContent to get the MIME type and then reject playback if it's not APPLICATION_OGG or AUDIO_WAV, but the best that'd do is reduce the chance of leaking somewhat as it's still possible that the Ogg or Wave is undecodable for various reasons.

Comment 12

[:Gijs (he/him)]

(In reply to Matthew Gregan [:kinetik] from comment #11)
> Thinking about what we can do until a fixed libcanberra is available: we can
> assume the leak has happened if ca_context_play returns CA_ERROR_CORRUPT,
> but the FILE* has been lost on the stack, so even if we dig the file
> descriptor out of /proc/self/fds and close it, we'd leak the FILE*.
> 
> We could use NS_SniffContent to get the MIME type and then reject playback
> if it's not APPLICATION_OGG or AUDIO_WAV, but the best that'd do is reduce
> the chance of leaking somewhat as it's still possible that the Ogg or Wave
> is undecodable for various reasons.

Does canberra not expose any other method than play() which tells you if canberra *can* actually play it, that doesn't leak the file pointer?

In the meantime, can consumers of nsISound work around by creating new instances and having the old ones be GC'd? I guess not?

Comment 13

[:Gijs (he/him)]

Alternatively, I wonder if for better support of various formats and so on, we shouldn't just make nsISound use the same backend as HTML5 <audio>, which will get us mp3 support "for free" on many platforms (including linux if compiled with gstreamer, I think?).

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to :Gijs Kruitbosch (Mostly gone until 22/9) from comment #12)
> Does canberra not expose any other method than play() which tells you if
> canberra *can* actually play it, that doesn't leak the file pointer?

Not that I'm aware of, but I'm not overly familiar with it.  The API is intentionally fairly simple: https://developer.gnome.org/libcanberra/unstable/libcanberra-canberra.html

> In the meantime, can consumers of nsISound work around by creating new
> instances and having the old ones be GC'd? I guess not?

Sadly not.

(In reply to :Gijs Kruitbosch (Mostly gone until 22/9) from comment #13)
> Alternatively, I wonder if for better support of various formats and so on,
> we shouldn't just make nsISound use the same backend as HTML5 <audio>, which
> will get us mp3 support "for free" on many platforms (including linux if
> compiled with gstreamer, I think?).

That's what bug 683263 proposes.

Comment 15

[Matthew Gregan [:kinetik]]

Couple of other potential workarounds:

- fork a new process to play the sound (insanely heavyweight, but does "solve" the leak)

- return an error from play() if we get CA_ERROR_CORRUPT and fix all the callers so that they can inform the user/disable the sound (minimizes the leak to once per sound file, more or less)

Comment 16

[Nicholas Nethercote [inactive]]

> - fork a new process to play the sound (insanely heavyweight, but does
> "solve" the leak)

Upstreaming the trivial fix and then getting that imported into Firefox is the best solution, right? Is this a significant enough bug that it's worth going to great efforts to work around it in the meantime?

Comment 17

[Matthew Gregan [:kinetik]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Upstreaming the trivial fix and then getting that imported into Firefox is
> the best solution, right? Is this a significant enough bug that it's worth
> going to great efforts to work around it in the meantime?

We don't ship libcanberra, so we'd either have to start or wait for distros to pick up the fix.

But yeah, given the situation, it's not a bug I plan on expending any further effort on.