1127213 - Firefox crashes on Windows after the changing default audio devices.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[tbolbat]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150128004009

Steps to reproduce:

Changed the default audio device in the playback devices tab in sound options.



Actual results:

Either it crashes right away after change OR it allows the change to happen but after a few times of changing the default audio device, it crashes the browser entirely.


Expected results:

Switch the current audio to the new default audio device.

Comment 1

[Matthew Gregan [:kinetik]]

Possibly a regression from bug 698079.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

On 37.0a2 (2015-01-28), I can't repro after swapping back-and-forth about 10 times while the first track on http://www.html5tutorial.info/html5-audio.php was playing.  The audio switched devices as the default was changed.

Comment 3

[tbolbat]

(In reply to Mark Hammond [:markh] from comment #2)
> On 37.0a2 (2015-01-28), I can't repro after swapping back-and-forth about 10
> times while the first track on http://www.html5tutorial.info/html5-audio.php
> was playing.  The audio switched devices as the default was changed.

I am on the same build and it just started to happen on my laptop. For my desktop however it's been doing this since the fix from bug 698079.

Comment 4

[(Away)]

Can you share some crash IDs from about:crashes? (assuming you have the crash reporter enabled)

Comment 5

[tbolbat]

bp-d0c59d26-6f96-46d6-9ccb-004aa2150129

Comment 6

[The009]

(In reply to David Major [:dmajor] (UTC+13) from comment #4)
> Can you share some crash IDs from about:crashes? (assuming you have the
> crash reporter enabled)

I am also getting the same issue. I am on 38.0a1 (2015-01-31). Win 7 x64

bp-7482ed0f-92c6-471c-8043-898802150201

Comment 7

[(Away)]

The crash is during the |delete| of a cubeb_resampler_speex. At first glace this is either an allocator mismatch or a bad/uninitialized resampler pointer. Any ideas?

bp-6c17a7eb-561d-46c2-834e-4ac5f2150128 may be related; that one looks like an uninitialized critical section.

Comment 8

[(Away)]

[Tracking Requested - why for this release]:
Given the uplift haste in bug 698079 this sounds like it's worth tracking for beta.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Assuming that this is indeed a regression from bug 698079 and MSE related, I take it that 36+ are affected. Tracking for 36+.

Comment 11

[Paul Adenot (:padenot)]

I've found and fixed some issues with the code today (including this crash and bug 1128371), and added some hardening (threading asserts mostly) to avoid both crashes and clock drifts. I'll attach the patch tomorrow, it needs some cleanup.

Comment 12

[u510688]

I have 3 audio out devices and this crash happens only for one of them. It seems to do with Equalizer APO, which is an EQ that uses Windows' Audio Processing Object API. The device with the crash is the only one with APO processing. Switching to this device while audio is playing crashes Firefox every time.

If I disable APO processing using the "Enable audio enhancements" option in the device's properties, Firefox no longer crashes when switching to it while audio is playing, but audio only comes out of the left channel when it should be stereo.

If APO processing is enabled ("Enable audio enhancements" ticked) and I select the troublesome audio device before playing audio, Firefox doesn't crash, but the audio stream immediately stops after pressing play.

The other devices which work correctly do have built-in effects enabled in the "Enhancements" tab.

None of the myriad other apps on my system have any issue with this device and work correctly with all of my audio devices (and switching between them on the fly), so the problem isn't with Equalizer APO itself.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Can you please share some of the crash IDs from about:crashes?

Comment 14

[u510688]

(In reply to Mark Hammond [:markh] from comment #13)
> Can you please share some of the crash IDs from about:crashes?

bp-1d6a417d-9063-49ff-874d-a81b02150204
bp-dcbbb068-5549-4f5a-8c77-860ac2150204
bp-7466012f-759b-4788-b905-4d11a2150204
bp-42769e36-cf0e-43a1-8f4a-15b8f2150204

Comment 15

[Paul Adenot (:padenot)]

Attachment: patch

This patch does the following:
- Introduces an owned_critical_section object to be able to assert that a current thread owns a critical section
- Change the auto_lock to use the above, add auto_unlock (basically like the ecko AutoEnter/AutoExit things)
- Fix an issue during audio output device switch where the clock would use the old sample rate. Apparently I did not notice this because my headset and the sound card on this laptop have the same rate
- Check that we could acquire a device_enumerator in the ctor before deallocating in the dtor, as that can happen if a ton of streams are running at once (I had this issue running the full mochitest suite)
- Stop getting another device_enumator in unregister_notification_client, fixing a leak
- Assert that setup_wasapi_stream and close_wasapi_stream are called with the lock held, this was the cause of the crash for this bug
- Make close_wasapi_stream clear out its state to make sure setup_wasapi_stream and close_wasapi_stream are called in the right order (especially, not two setup_wasapi_stream without close in between, since that would leak stuff)
- in wasapi_stream_destroy, unregister the notification client before destroying the CRITICAL_SECTION (this was the cause of a crash I duped against this bug)

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8559176 [details] [diff] [review]
patch

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

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +30,5 @@
>  #ifndef STACK_SIZE_PARAM_IS_A_RESERVATION
>  #define STACK_SIZE_PARAM_IS_A_RESERVATION   0x00010000    // Threads only
>  #endif
>  
> +#define LOGGING_ENABLED

I'll remove this.

Comment 17

[Marcia Knous [:marcia]]

Adding the crash signature for tracking.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

(In reply to [e-mail] from comment #14)
> (In reply to Mark Hammond [:markh] from comment #13)
> > Can you please share some of the crash IDs from about:crashes?
> 
> bp-1d6a417d-9063-49ff-874d-a81b02150204
> bp-dcbbb068-5549-4f5a-8c77-860ac2150204
> bp-7466012f-759b-4788-b905-4d11a2150204
> bp-42769e36-cf0e-43a1-8f4a-15b8f2150204

Awesome, thanks!  These have the same crash signature, so should also be resolved in this bug.

Comment 19

[Matthew Gregan [:kinetik]]

Comment on attachment 8559176 [details] [diff] [review]
patch

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

r+ with comments addressed

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +1085,5 @@
>      return CUBEB_ERROR;
>    }
>  
> +  {
> +    // Locking here is not stricly necessary, because we don't have a

typo

@@ +1116,2 @@
>  
> +  if (stm->client) {

SafeRelease already tests for nullptr.

@@ +1121,2 @@
>  
> +  if (stm->render_client) {

Ditto.

@@ +1128,5 @@
> +    cubeb_resampler_destroy(stm->resampler);
> +    stm->resampler = nullptr;
> +  }
> +
> +  if (stm->mix_buffer) {

free(nullptr) is valid, so this is unnecessary.

Comment 20

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f46afa97421

Comment 21

[Paul Adenot (:padenot)]

Comment on attachment 8559176 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:  698079
[User impact if declined]: crash 
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: this adds some hardening features, and is needed for MSE I believe
[String/UUID change made/needed]: none

Comment 22

[Carsten Book [:Tomcat]]

had to backout this too for another bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6408280&repo=mozilla-inbound

Comment 23

[Paul Adenot (:padenot)]

Relanded, I confused assert() and MOZ_ASSERT().

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9e5849856d

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8559176 [details] [diff] [review]
patch

Taking it to make sure it is in beta 8

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad9e5849856d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8df96693f4f

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9579b9ab68ca

Comment 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/c939ab0df211

Comment 29

[[e-mail]]

(In reply to Paul Adenot from comment #15)
> - in wasapi_stream_destroy, unregister the notification client

This is probably the cause of the crash that my local build (which predates this patch) just got when switching audio devices from Remote Desktop to whatever the built-in audio is on my PC. Here's the stack annotated with relevant nearby source lines:

xul!cubeb_resampler_destroy+0x11
  delete resampler;
xul!`anonymous namespace'::close_wasapi_stream+0x52
  cubeb_resampler_destroy(stm->resampler);
xul!`anonymous namespace'::wasapi_stream_destroy+0x88
  unregister_notification_client(stm);
xul!`anonymous namespace'::setup_wasapi_stream+0x1e3
  if (!stm->resampler) {
    LOG("Could not get a resampler");
    wasapi_stream_destroy(stm);
xul!wasapi_endpoint_notification_client::OnDefaultDeviceChanged+0x68
MMDevApi!CDeviceEnumerator::OnDefaultDeviceChanged+0x8f
MMDevApi!CDeviceEnumerator::OnMainEnumeratorNotification+0x81
MMDevApi!CDeviceEnumerator::OnMediaNotification+0xd5
MMDevApi!CMediaNotifications::OnMediaNotificationWorkerHandler+0x2e2
ntdll!TppSimplepExecuteCallback+0x91
ntdll!TppWorkerThread+0x5ff
kernel32!BaseThreadInitThunk+0xd
ntdll!RtlUserThreadStart+0x1d

Note that at the 4th frame, resampler is null, but at the top frame, it turns out that resampler is pointing at deleted memory.

Comment 30

[Luke]

https://hg.mozilla.org/mozilla-central/rev/2f46afa97421

Is causing a regression where the sound subsystem cuts out under heavy load. See Bug 1134977