1358756 - Crash in `anonymous namespace''::refill

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-20b6ff11-8283-4f26-b701-c0de80170421.
=============================================================

There are 11 crashes in nightly 55 with buildid 20170421030241. In analyzing the backtrace the regression may have been introduced by patch [1] to fix bug 1357683.

[1] https://hg.mozilla.org/mozilla-central/rev?node=819e873822636fb84c3d6ee8f933998e1892edf5

Comment 1

[Alex Chronopoulos [:achronop]]

Crash on commit: https://github.com/kinetiknz/cubeb/commit/727a7a4bccd046e9c411e030bcc34390eb5644ed

Comment 2

[Nicholas Nethercote [inactive]]

The crash address is always 0x0. Perhaps |stm->mixing| is null?

Comment 3

[Paul Adenot (:padenot)]

Chun-Min, do you have a minute to look at this? I can take it otherwise.

Comment 4

[C.M.Chang[:chunmin]]

Sure.

Comment 5

[C.M.Chang[:chunmin]]

I guess stm->mixing->mix is called after close_wasapi_stream or before wasapi_stream_init. If there is a callback thread executed after close_wasapi_stream on another thread, then stm->mixing will be empty.

Comment 6

[Matthew Gregan [:kinetik]]

A close_wasapi_stream and setup_wasapi_stream will happen on a device change.  That's a highly likely path to be hit and almost certainly the explanation for the crash.

The current code is definitely wrong to initialize a resource in wasapi_stream_init and destroy it in close_wasapi_stream.  Either it should be destroyed in wasapi_stream_destroy (if it doesn't rely on any stream config specific state, which in this case it doesn't appear to) or it should be initialized in setup_wasapi_stream to pick up any config changes.

So fixing this is probably as simple as moving the reset() of stm->mixing from close_wasapi_stream to wasapi_stream_destroy.

Looks like this also applies to stm->linear_input_buffer, which has the same mis-matched creation/destruction pairing in wasapi_stream_init and close_wasapi_stream.

Comment 7

[C.M.Chang[:chunmin]]

(In reply to Matthew Gregan [:kinetik] from comment #6)
> A close_wasapi_stream and setup_wasapi_stream will happen on a device
> change.  That's a highly likely path to be hit and almost certainly the
> explanation for the crash.
> 
> The current code is definitely wrong to initialize a resource in
> wasapi_stream_init and destroy it in close_wasapi_stream.  Either it should
> be destroyed in wasapi_stream_destroy (if it doesn't rely on any stream
> config specific state, which in this case it doesn't appear to) or it should
> be initialized in setup_wasapi_stream to pick up any config changes.
> 
> So fixing this is probably as simple as moving the reset() of stm->mixing
> from close_wasapi_stream to wasapi_stream_destroy.
> 
> Looks like this also applies to stm->linear_input_buffer, which has the same
> mis-matched creation/destruction pairing in wasapi_stream_init and
> close_wasapi_stream.
Yes, it crashes when device is switched. Thanks for the hints!

This notice me another question:
Is callback allowed to run after stream is destroyed? The test[0] shows there is a crash in this situation.

[0] https://github.com/ChunMinChang/cubeb/commit/53cbf00c1f46cc6e2e4b8987e67f41bb8b1d076b#diff-173dd644c9b7abe6868416daaf650d20

Comment 8

[C.M.Chang[:chunmin]]

Attachment: pull on github

Comment 9

[C.M.Chang[:chunmin]]

Merged in https://github.com/kinetiknz/cubeb/pull/292

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Shouldn't we wait to close this bug until the fix makes it to m-c?

Comment 11

[C.M.Chang[:chunmin]]

ok, we could close it after we don't see it anymore.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

No crashes with build dates after bug 1362334 merged to central and got into nightly