Closed Bug 1358756 Opened 5 years ago Closed 5 years ago

Crash in `anonymous namespace''::refill

Categories

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

55 Branch
Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: calixte, Assigned: chunmin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

43 bytes, text/x-github-pull-request
Details | Review
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
Flags: needinfo?(achronop)
Crash on commit: https://github.com/kinetiknz/cubeb/commit/727a7a4bccd046e9c411e030bcc34390eb5644ed
Flags: needinfo?(padenot)
Flags: needinfo?(kinetik)
The crash address is always 0x0. Perhaps |stm->mixing| is null?
Chun-Min, do you have a minute to look at this? I can take it otherwise.
Flags: needinfo?(padenot) → needinfo?(cchang)
Rank: 15
Priority: -- → P1
Sure.
Flags: needinfo?(cchang)
Flags: needinfo?(kinetik)
Flags: needinfo?(achronop)
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.
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.
(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
Attached file pull on github
Assignee: nobody → cchang
Merged in https://github.com/kinetiknz/cubeb/pull/292
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Shouldn't we wait to close this bug until the fix makes it to m-c?
Flags: needinfo?(cchang)
ok, we could close it after we don't see it anymore.
Status: RESOLVED → REOPENED
Flags: needinfo?(cchang)
Resolution: FIXED → ---
Depends on: 1362334
No crashes with build dates after bug 1362334 merged to central and got into nightly
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.