Firefox crashes on Windows after the changing default audio devices.

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tbolbat, Assigned: padenot)

Tracking

({crash})

37 Branch
mozilla38
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36+ fixed, firefox37+ fixed, firefox38+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Possibly a regression from bug 698079.
Blocks: 698079
Reporter

Updated

4 years ago
Summary: FIrefox crashes on Windows after the changing default audio devices. → Firefox crashes on Windows after the changing default audio devices.
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.
Reporter

Comment 3

4 years ago
(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.
Can you share some crash IDs from about:crashes? (assuming you have the crash reporter enabled)
Flags: needinfo?(tbolbat)
Reporter

Comment 5

4 years ago
bp-d0c59d26-6f96-46d6-9ccb-004aa2150129
Flags: needinfo?(tbolbat)

Comment 6

4 years ago
(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
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.
Flags: needinfo?(padenot)
Flags: needinfo?(kinetik)
[Tracking Requested - why for this release]:
Given the uplift haste in bug 698079 this sounds like it's worth tracking for beta.
Keywords: crash
Assuming that this is indeed a regression from bug 698079 and MSE related, I take it that 36+ are affected. Tracking for 36+.
Assignee

Updated

4 years ago
Duplicate of this bug: 1128371
Assignee

Comment 11

4 years ago
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.
Flags: needinfo?(padenot)
Assignee

Updated

4 years ago
Assignee: nobody → padenot
Flags: needinfo?(kinetik)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 12

4 years ago
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.
Can you please share some of the crash IDs from about:crashes?
Flags: needinfo?(hararghost)

Comment 14

4 years ago
(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
Flags: needinfo?(hararghost)
Assignee

Comment 15

4 years ago
Posted patch patchSplinter Review
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)
Attachment #8559176 - Flags: review?(kinetik)
Assignee

Comment 16

4 years ago
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.
Adding the crash signature for tracking.
Crash Signature: [@ RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*) ]
Crash Signature: [@ RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*) ] → [@ RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)] [@ RtlpWaitOnCriticalSection | EtwEventEnabled | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)]
(In reply to hararghost@hotmail.com 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 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.
Attachment #8559176 - Flags: review?(kinetik) → review+
Assignee

Comment 21

4 years ago
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
Attachment #8559176 - Flags: approval-mozilla-beta?
Attachment #8559176 - Flags: approval-mozilla-aurora?
had to backout this too for another bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6408280&repo=mozilla-inbound
Flags: needinfo?(padenot)
Assignee

Comment 23

4 years ago
Relanded, I confused assert() and MOZ_ASSERT().

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9e5849856d
Flags: needinfo?(padenot)
Comment on attachment 8559176 [details] [diff] [review]
patch

Taking it to make sure it is in beta 8
Attachment #8559176 - Flags: approval-mozilla-beta?
Attachment #8559176 - Flags: approval-mozilla-beta+
Attachment #8559176 - Flags: approval-mozilla-aurora?
Attachment #8559176 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/ad9e5849856d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Crash Signature: [@ RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)] [@ RtlpWaitOnCriticalSection | EtwEventEnabled | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)] → [@ RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)] [@ RtlpWaitOnCriticalSection | EtwEventEnabled | `anonymous namespace''::wasapi_stream_stop(cubeb_stream*)] [@ jemalloc_crash | je_free | `…
Depends on: 1131768

Updated

4 years ago
Depends on: 1133306
Depends on: 1133386

Updated

4 years ago
Depends on: 1132034
(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

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

Is causing a regression where the sound subsystem cuts out under heavy load. See Bug 1134977
Depends on: 1135878
You need to log in before you can comment on or make changes to this bug.