Closed
Bug 1131768
Opened 10 years ago
Closed 10 years ago
crash in _wassert
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox35 | --- | unaffected |
firefox36 | + | verified |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
People
(Reporter: away, Assigned: padenot)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
(deleted),
patch
|
kinetik
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
[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.
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.
Flags: needinfo?(padenot)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(kinetik)
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.
Flags: needinfo?(mh+mozilla)
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);
}
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → padenot
Flags: needinfo?(padenot)
> 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.
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8562405 -
Flags: review?(kinetik)
Updated•10 years ago
|
Attachment #8562405 -
Flags: review?(kinetik) → review+
Updated•10 years ago
|
Flags: needinfo?(kinetik)
Comment 8•10 years ago
|
||
Comment 9•10 years ago
|
||
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
Attachment #8562405 -
Flags: approval-mozilla-beta?
Attachment #8562405 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•10 years ago
|
Attachment #8562405 -
Flags: approval-mozilla-beta?
Attachment #8562405 -
Flags: approval-mozilla-beta+
Attachment #8562405 -
Flags: approval-mozilla-aurora?
Attachment #8562405 -
Flags: approval-mozilla-aurora+
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
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•10 years ago
|
||
Looks to me like https://crash-stats.mozilla.com/report/list?signature=wassert is probably the Win64 equivalent of this.
Crash Signature: [@ _wassert] → [@ _wassert]
[@ wassert]
Updated•10 years ago
|
Flags: qe-verify+
Comment 16•10 years ago
|
||
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?
Flags: needinfo?(padenot)
Assignee | ||
Comment 17•10 years ago
|
||
It should be fine for now. In any case, those '@ wassert' crashes don't look super actionable from here.
Flags: needinfo?(padenot)
Updated•10 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 18•10 years ago
|
||
FWIW many of those remaining wassert's will change signatures after bug 1133386.
You need to log in
before you can comment on or make changes to this bug.
Description
•