934232 - Assertion failure in pa_stream_set_state_callback()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Brian Gomes Bascoy]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130917081302

Steps to reproduce:

Tried to load many different songs quickly using mod.js (http://duckinator.net/mod.js/) that is probably using the already deprecated Audio Data API.


Actual results:

Firefox crashed after an assertion failure from PulseAudio:

Assertion 's' failed at pulse/stream.c:2089, function pa_stream_set_state_callback(). Aborting.
Aborted (core dumped)



Expected results:

I believe (didnt debugged anything) the origin of the error is in /source/media/libcubeb/src/cubeb_pulse.c line 504:

stm->stream = WRAP(pa_stream_new)(stm->context->context, stream_name, &ss, &map);
WRAP(pa_stream_set_state_callback)(stm->stream, stream_state_callback, stm);

Since pa_stream_set_state_callback() checks if the first parameter ('s') is not 0, so you should check first what pa_stream_new is returning.

Comment 1

[Matthew Gregan [:kinetik]]

I don't think this needs to be sec-sensitive, it's a simple assertion failure.

Comment 2

[Matthew Gregan [:kinetik]]

Thanks for the bug report and analysis, you're completely correct that pa_stream_new failures should be handled.

I've pushed a fix here: https://tbpl.mozilla.org/?tree=Try&rev=4e9f1d7629bf

Builds will turn up here once they're done: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4e9f1d7629bf/

Can you please test the resulting build and let me know how it goes?  I'm curious about why pa_stream_new is failing on your system, so I've added some PA error logging to stderr too.

Comment 3

[Brian Gomes Bascoy]

Thanks for your reply Matthew, your fix looks alright and should work, I'll try it tonight.
The reason I sent this to core_security is because the assert only checks if 's' is not 0: my concern was that maybe that's not enough and some more advanced validation is needed... but never mind, it was just an unfounded feeling.

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to pera from comment #3)
> Thanks for your reply Matthew, your fix looks alright and should work, I'll
> try it tonight.

Thanks, I fear the result is that it'll fix the crash but you'll get no audio now, so there may be a secondary issue we need to get to the bottom of.

> The reason I sent this to core_security is because the assert only checks if
> 's' is not 0: my concern was that maybe that's not enough and some more
> advanced validation is needed... but never mind, it was just an unfounded
> feeling.

It's not a problem, it doesn't hurt to be careful!

Comment 5

[Matthew Gregan [:kinetik]]

Did you have a chance to test this yet?

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: bug934232_v0.patch

The crash fix is simple, so let's take that and deal with any other problems in a new bug.

Comment 7

[Matthew Gregan [:kinetik]]

FWIW, I removed the pa_channel_map_init_auto because passing pa_stream_new NULL does the same thing.

Comment 8

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd5efa062a5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fd5efa062a5

Comment 10

[u279076]

pera, can you please verify this is fixed in the latest Aurora builds?

Comment 11

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> pera, can you please verify this is fixed in the latest Aurora builds?

Canceling this long overdue request.