1131768 - crash in _wassert

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-50112851-3d94-44ab-83d7-5931d2150210.
=============================================================

0 	msvcr120.dll 	_wassert 	f:\dd\vctools\crt\crtw32\misc\assert.c:369
1 	xul.dll 	`anonymous namespace'::owned_critical_section::enter() 	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 	`anonymous namespace'::wasapi_stream_init(cubeb*, cubeb_stream**, char const*, cubeb_stream_params, unsigned int, long (*)(cubeb_stream*, void*, void*, long), void (*)(cubeb_stream*, void*, cubeb_state), void*) 	media/libcubeb/src/cubeb_wasapi.cpp
5 	xul.dll 	cubeb_stream_init 	media/libcubeb/src/cubeb.c
6 	xul.dll 	mozilla::AudioStream::DataCallback(void*, long) 	dom/media/AudioStream.cpp
7 	xul.dll 	mozilla::AudioStream::Init(int, int, mozilla::dom::AudioChannel, mozilla::AudioStream::LatencyRequest) 	dom/media/AudioStream.cpp
8 	xul.dll 	mozilla::AudioSink::InitializeAudioStream() 	dom/media/AudioSink.cpp
9 	xul.dll 	mozilla::AudioSink::AudioLoop() 	dom/media/AudioSink.cpp
10 	nss3.dll 	_MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c
11 	xul.dll 	nsRunnableMethodImpl<void ( mozilla::crashreporter::LSPAnnotationGatherer::*)(void), void, 1>::Run() 	xpcom/glue/nsThreadUtils.h
12 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
13 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
14 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc

Comment 1

[(Away)]

[Tracking Requested - why for this release]: This is a top crash in very early 36b8 data (5 crashes out of 114). Normally that little data is too noisy, but given that it's a new regression with little time left, this is worth keeping an eye on.

Comment 2

[(Away)]

The assert is:

assert(owner != GetCurrentThreadId() && "recursive locking");

For one thing I am puzzled that this is active in a release build. The |assert| family is disabled by the NDEBUG symbol which we do define. Here is the about:buildconfig from 36b8:

Compiler 	Version 	Compiler flags
cl 	1800 	-TC -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4244 -wd4267 -wd4819 -we4553
cl 	1800 	-TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 -Oi -Oy 

(Why do we have two different lines there?) And does cubeb do its own custom flags or anything?

In any case it still sounds like a bug that we mismatched threads, regardless of whether the assert fired.

Comment 3

[(Away)]

Waitasec, this code is behind  #ifdef DEBUG. What the?

Comment 4

[(Away)]

Oh I see...

    1.10 +// This enables assert in release, and lets us have debug-only code
    1.11 +#ifdef NDEBUG
    1.12 +#define DEBUG
    1.13  #undef NDEBUG
    1.14 +#endif // #ifdef NDEBUG

Ok, one mystery solved, but still that's kind of scary, especially wrt unified builds.

Comment 5

[(Away)]

Ok so this seems to be:

wasapi_stream_init:
    auto_lock lock(stm->stream_reset_lock);
    rv = setup_wasapi_stream(stm);

Then setup_wasapi_stream:
  if (!stm->resampler) {
    LOG("Could not get a resampler");
    wasapi_stream_destroy(stm);
    return CUBEB_ERROR;
  }

Then wasapi_stream_destroy:
  {
    auto_lock lock(stm->stream_reset_lock);    <-- locks again
    close_wasapi_stream(stm);
  }

Comment 6

[(Away)]

> Normally that little data is too noisy
With more data coming in this is up to the #1 spot at 12% of b8 crashes. Looks like it's real.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Unlock before tearing down the stream in case of error on setup, to avoid reccursive locking. r=

Untested, because I don't have a windows builder handy. I'm quite confused as
why the resampler init fails, and I'll look at that again tomorrow morning first
thing.

Maybe there is a sound file out there that has a very very low intrinsic
sample-rate, and people are using high sample-rate output devices, so we end up
allocating a ton of memory, but given that :dmajor told us there is quite a lot
of memory available (2.8GB on the crash report linked), I'm not sure.

In any case, we can also backout 20f7d44346da, 0411d20465b4, and 9579b9ab68ca.

Comment 8

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72823d7a5250

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8562405 [details] [diff] [review]
Unlock before tearing down the stream in case of error on setup, to avoid reccursive locking. r=

Approval Request Comment
[Feature/regressing bug #]: bug 1127213
[User impact if declined]: crash in OOM/audio device error situations
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low, only change is dropping lock on error path
[String/UUID change made/needed]: none

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72823d7a5250

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/69a8d311ddd6

Comment 13

[Karl Tomlinson (:karlt)]

I was seeing this frequently immediately after seeking on https://www.youtube.com/watch?v=9aMYvNCXA_I (with or without MSE, 480 or 720 lines).  Selecting "Ignore" in the assertion failure dialog would leave the video playing with no audio until the next successful seek.

Comment 15

[Robert Kaiser]

Looks to me like https://crash-stats.mozilla.com/report/list?signature=wassert is probably the Win64 equivalent of this.

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Socorro shows no crashes recorded in Firefox 36 beta with '@ _wassert' signature after the fix, marking this as verified fixed.

https://crash-stats.mozilla.com/search/?signature=~_wassert&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Although there are still crashes recorded on latest Nightly with '@ wassert' signature.

https://crash-stats.mozilla.com/signature/?signature=wassert&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1

Is this something that we should worry about?

Comment 17

[Paul Adenot (:padenot)]

It should be fine for now. In any case, those '@ wassert' crashes don't look super actionable from here.

Comment 18

[(Away)]

FWIW many of those remaining wassert's will change signatures after bug 1133386.