1128649 - Firefox crash in @0x0 | `anonymous namespace''::close_wasapi_stream(cubeb_stream*)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-99760422-9a35-4f9c-8f2a-9ca6d2150202.
=============================================================

Seen while looking at crash stats - almost all URLs are youtube.com.

Link to all reports: https://crash-stats.mozilla.com/report/list?signature=@0x0%20|%20%60anonymous%20namespace%27%27::close_wasapi_stream%28cubeb_stream*%29.

Small volume crash which started showing up first using Build ID=2015012203.


Frame 	Module 	Signature 	Source
0 		@0x0 	
1 	xul.dll 	`anonymous namespace'::close_wasapi_stream(cubeb_stream*) 	media/libcubeb/src/cubeb_wasapi.cpp
2 	xul.dll 	`anonymous namespace'::wasapi_stream_destroy(cubeb_stream*) 	media/libcubeb/src/cubeb_wasapi.cpp
3 	xul.dll 	`anonymous namespace'::setup_wasapi_stream(cubeb_stream*) 	media/libcubeb/src/cubeb_wasapi.cpp
4 	xul.dll 	wasapi_endpoint_notification_client::OnDefaultDeviceChanged(__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0001, __MIDL___MIDL_itf_mmdeviceapi_0000_0000_0002, wchar_t const*) 	media/libcubeb/src/cubeb_wasapi.cpp
5 	mmdevapi.dll 	CDeviceEnumerator::OnDefaultDeviceChanged(__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0001, __MIDL___MIDL_itf_mmdeviceapi_0000_0000_0002, unsigned short const*) 	
6 	mmdevapi.dll 	_chkstk 	
7 	mmdevapi.dll 	ATL::CAtlArray<ATL::CComQIPtr<IPnpNotificationClient, &__s_GUID const _GUID_11baae68_d8ee_45b4_b541_97d517a6f57a>, ATL::CComQIPtrElementTraits<IPnpNotificationClient, &__s_GUID const _GUID_11baae68_d8ee_45b4_b541_97d517a6f57a> >::~CAtlArray<ATL::CComQIPtr<IPnpNotificationClient, &__s_GUID const _GUID_11baae68_d8ee_45b4_b541_97d517a6f57a>, ATL::CComQIPtrElementTraits<IPnpNotificationClient, &__s_GUID const _GUID_11baae68_d8ee_45b4_b541_97d517a6f57a> >() 	
8 	d2d1.dll 	RecordThreadTraceEvent(ThreadTraceEvent::Enum, unsigned long, int, PCChainManager*) 	
9 	ntdll.dll 	TppWorkCallbackPrologRelease 	
10 	ntdll.dll 	RtlAllocateMemoryBlockLookaside 	
11 	ntdll.dll 	TppSimplepExecuteCallback 	
12 	ntdll.dll 	RtlAllocateMemoryBlockLookaside 	
13 	ntdll.dll 	RtlRealSuccessor 	
14 	ntdll.dll 	RtlAllocateMemoryBlockLookaside 	
15 	ntdll.dll 	RtlAllocateMemoryBlockLookaside 	
16 	ntdll.dll 	RtlAllocateMemoryBlockLookaside

Comment 1

[Matthew Gregan [:kinetik]]

Regression from bug 698079.  Hopefully Paul's fixes in bug 1127213 will resolve this.

Comment 2

[Marcia Knous [:marcia]]

will recheck this now that bug 1127213 landed.

Comment 3

[Marcia Knous [:marcia]]

For the first signature, I do see a single crash with today's build ID in this signature on Aurora: https://crash-stats.mozilla.com/report/index/b4488e87-6952-4a01-9159-1a2cc2150212. The second signature in the crash signature field has not been seen since 2015020916.

The big spike in this signature was using 2015020218, and since then I don't see any large volume in crash-stats.

Comment 4

[Matthew Gregan [:kinetik]]

This looks like the resampler (re-)allocation failing during a device change, possible due to unusual sample rate differences?

One thing I did notice is that close_wasapi_stream calls SafeRelease(stm->render_client), but then sets stm->client = nullptr rather than stm->render_client.  That's wrong, but I can't see how it's involved in this crash since we've already reached the resampler allocation, so stm->render_client should be a new valid value from the call to GetService (and if that had failed, it should've set stm->render_client to null and then we'd be in the GetService error handling path).

Paul, any ideas on the resampler issue?

Comment 5

[Paul Adenot (:padenot)]

Attachment: Properly null-out render_client in close_wasapi_stream. r=

I'm still digging, but good catch.

Comment 6

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

Tracking all MSE P1 bugs for Firefox 37.

Comment 7

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #4)
> This looks like the resampler (re-)allocation failing during a device
> change, possible due to unusual sample rate differences?
> 
> One thing I did notice is that close_wasapi_stream calls
> SafeRelease(stm->render_client), but then sets stm->client = nullptr rather
> than stm->render_client.  That's wrong, but I can't see how it's involved in
> this crash since we've already reached the resampler allocation, so
> stm->render_client should be a new valid value from the call to GetService
> (and if that had failed, it should've set stm->render_client to null and
> then we'd be in the GetService error handling path).
> 
> Paul, any ideas on the resampler issue?

I caught this on a Nightly (that is, without the patch for bug 1133190) in a debugger, and the found something weird: at the call site for cubeb_resampler_create, the value for the output rate was 48000Hz, and in the function, it was 0, so either something got optimized away (and the VS debugger failed to inform me), or some memory got trashed (presumably because we tried to unlock a freed mutex 40 times right before crashing here, and because the resulting allocated memory can still be intact, so it does not crash every time). In any case, this output rate of 0 is something that the resampler refuses to deal with and errors out, and then the code crashes on a variant of bug 1133190, since it tries to relock a freed mutex.

I've tried to repro with a nightly-like (same mozconfig) build that has the patch for bug 1133190, without success. I want to try a couple other things before calling this fixed, though.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8565040 [details] [diff] [review]
Properly null-out render_client in close_wasapi_stream. r=

This was landed via bug 1132257.

Comment 9

[Sheila Mooney]

We don't see any of these anymore. We assume it's fixed.

Comment 10

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

Marking 38 as fixed as that's where bug 1132257 landed.

37 is marked as affected. Do we need to uplift the fix in bug 1132257 to 37?

Comment 11

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

I'm going to answer my own ni. I don't see any crashes on 37 since 37 was on Aurora. Marking 37 as fixed.