Closed Bug 1550695 Opened 4 months ago Closed 3 months ago

Crash in [@ wasapi_init]

Categories

(Core :: Audio/Video: cubeb, defect, P2, critical)

68 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: calixte, Assigned: kinetik)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

This bug is for crash report bp-96b0b48c-863c-4847-aa35-05f540190510.

Top 10 frames of crashing thread:

0 xul.dll wasapi_init media/libcubeb/src/cubeb_wasapi.cpp:1452
1 xul.dll cubeb_init media/libcubeb/src/cubeb.c:216
2 xul.dll static struct cubeb* mozilla::CubebUtils::GetCubebContextUnlocked dom/media/CubebUtils.cpp:493
3 xul.dll mozilla::CubebUtils::GetCubebContext dom/media/CubebUtils.cpp:310
4 xul.dll mozilla::CubebUtils::MaxNumberOfChannels dom/media/CubebUtils.cpp:640
5 xul.dll mozilla::dom::AudioContext::Constructor dom/media/webaudio/AudioContext.cpp:279
6 xul.dll static bool mozilla::dom::AudioContext_Binding::_constructor dom/bindings/AudioContextBinding.cpp:648
7 xul.dll static bool InternalConstruct js/src/vm/Interpreter.cpp:652
8 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3073
9 xul.dll js::RunScript js/src/vm/Interpreter.cpp:423

There are 133 crashes in nightly 68 with buildid >= 20190408104625 (so for builds containing patch for bug 1541805).
:padenot, could you investigate please?

Flags: needinfo?(padenot)

We're instantiating an AudioContext from the main thread, this is initializing cubeb, which tries to find at least a device that we can use here. This needs an initialized COM. It's a variant on bug 1449555, one level higher.

Matthew, I think it's time to initialize cubeb off-main-thread, I don't remember all the details yet, but iirc doing this on the main thread causes architectural issues with remoting?

Flags: needinfo?(padenot) → needinfo?(kinetik)

Also the rather low count of crashes might be because of this: https://devblogs.microsoft.com/oldnewthing/20130419-00/?p=4613.

Using or initializating libcubeb from the main thread has been an issue for quite a while - we have a bug (bug 1430704) to fix that. Moving libcubeb use (or even just initialization) off of the main thread would make remoting easier and more robust. The Gecko main thread is used for remoting initialization (because we use Gecko IPC to bootstrap) - moving libcubeb initialization off of the main thread makes remoting initialization more robust because we can wait for the main thread IPC to complete off-thread.

Flags: needinfo?(kinetik)
Priority: -- → P2
Keywords: regression

It sounds to me like moving the initialization to another thread is a bigger project, not ideal for uplifting. Is there anything we could do in between to avoid these crashes?

Flags: needinfo?(padenot)
Flags: needinfo?(kinetik)

We can do a quick hack:

  • get a monitor
  • start a thread
  • block the main thread on the monitor
  • init cubeb on this thread from an MTA appartment
  • signal the monitor
  • destroy the thread
Flags: needinfo?(padenot)

Paul's idea reminded me that we already have a solution for this in the tree - EnsureMTA. We may need to wrap any libcubeb call that can originate from the main thread with EnsureMTA, not just initialization, as it's probably bad to use our MTA-initialized COM objects on threads with indeterminate COM initialization state.

Flags: needinfo?(kinetik)
Assignee: nobody → kinetik
Status: NEW → ASSIGNED

I can't be certain this patch fixes the core issue since I don't have an explanation for how we end up in the situation where COM is uninitialized in the thread calling cubeb_init or a way of reproducing the crash. I've tested various main and off-main thread scenarios locally with logging and verified the change does what I expect. Just waiting on some Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f21d8130bb39305e46ab9a3e7052f8c0d0b5e1

Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a88deceba11
Avoid duplicating cubeb_init in both arms of MOZ_CUBEB_REMOTING ifdef.  r=achronop
https://hg.mozilla.org/integration/autoland/rev/546170ac7ca9
Wrap cubeb_init in EnsureMTA to handle being called from threads where COM is uninitialized.  r=achronop
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9067673 [details]
Bug 1550695 - Wrap cubeb_init in EnsureMTA to handle being called from threads where COM is uninitialized. r?achronop

Beta/Release Uplift Approval Request

  • User impact if declined: Potential crash when using WebAudio (or anything else that could trigger main thread audio initialization).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Change is fairly simple/safe - uses existing mechanism (EnsureMTA) to guarantee initialization runs in the required context.
  • String changes made/needed:
Attachment #9067673 - Flags: approval-mozilla-beta?
Attachment #9067672 - Flags: approval-mozilla-beta?

Comment on attachment 9067673 [details]
Bug 1550695 - Wrap cubeb_init in EnsureMTA to handle being called from threads where COM is uninitialized. r?achronop

fix crash in webaudio, approved for 68.0b7

Attachment #9067673 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9067672 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.