1884214 - Crash in [@ __delayLoadHelper2 | _tailMerge_avrt.dll | audio_thread_priority::rt_win::promote_current_thread_to_real_time_internal]

Status: ASSIGNED

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

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.org/report/index/9235dfd4-32ca-4a40-a4e6-4337e0240307

Reason: FACILITY_VISUALCPP / ERROR_MOD_NOT_FOUND

Top 10 frames of crashing thread:

0  KERNELBASE.dll  RaiseException  
1  xul.dll  __delayLoadHelper2  /builds/worker/workspace/obj-build/toolkit/library/build/D:/a/_work/1/s/src/vctools/delayimp/delayhlp.cpp:301
2  xul.dll  _tailMerge_avrt.dll  
3  xul.dll  audio_thread_priority::rt_win::promote_current_thread_to_real_time_internal  third_party/rust/audio_thread_priority/src/rt_win.rs:53
3  xul.dll  audio_thread_priority::promote_current_thread_to_real_time  third_party/rust/audio_thread_priority/src/lib.rs:471
4  xul.dll  audioipc2_server::init_threads::closure$3  third_party/rust/audioipc2-server/src/lib.rs:101
4  xul.dll  audioipc2::ipccore::impl$7::new::closure$0  third_party/rust/audioipc2/src/ipccore.rs:701
4  xul.dll  std::sys_common::backtrace::__rust_begin_short_backtrace<audioipc2::ipccore::impl$7::new::closure_env$0<audioipc2_server::init_threads::closure_env$3, audioipc2_server::init_threads::closure_env$4>, enum2$<core::result::Result<tuple$<>, std::io::error::Error> > >  /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:155
5  xul.dll  std::thread::impl$0::spawn_unchecked_::closure$1::closure$0  /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/thread/mod.rs:527
5  xul.dll  core::panic::unwind_safe::impl$23::call_once  /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272

Comment 1

[Alastor Wu [:alwu][PTO 8/28-9/6]]

Hi, kinetik, this is an audio IPC issue, would you mind to take a look on this? Thanks.

Comment 2

[Paul Adenot (:padenot)]

This is in audio_thread_priority, clearing the NI to kinetik.

Bob, Yannis, this is crashing here: https://searchfox.org/mozilla-central/source/third_party/rust/audio_thread_priority/src/rt_win.rs#53. This is just calling the avrt.dll function of the same name. The crash says: FACILITY_VISUALCPP / ERROR_MOD_NOT_FOUND, is this something we can sidestep on our side? This is generally working, but there's been a low volume crash for some time.

Searching for that error online, I see weird messages about ffmpeg.dll installed as a system lib (!?), and this also happening on Chrome, but hard to know what this is about.

Comment 3

[Yannis Juglaret [:yannis]]

The way we call into AvSetMmThreadCharacteristicsA (and other avrt.dll functions) makes xul.dll depend on avrt.dll. We make this dependency use delay-loading, i.e. load the DLL at function call site, due to some sandboxing requirements (see bug 1546154). The crash that we see here is caused by failing to load the DLL at the function call site. It is normal and expected that failure to delay-load a DLL results in a crash.

If there were only child process crashes here, this failure could indicate an issue with sandboxing where the function call would occur too late (after lowering the sandbox). But since there are parent process crashes as well, it seems that it's just not safe in general to rely on the availability of this specific DLL on all user machines.

In order to avoid crashing here, we would need to make sure that we never call into an avrt.dll function unless we have first sucessfully loaded the library with a common manual call to LoadLibrary. In addition to that, we could also consider using GetProcAddress rather than delay-loading for interfacing with this library (like webrtc code does it), which would be a more standard way to depend on a library that can fail to load.

Depending on the importance of these calls, we may also want to add some telemetry about LoadLibrary failures for this DLL.

Comment 4

[Yannis Juglaret [:yannis]]

Bug 1885501 should fix the issue in audio_thread_priority and dependents. There is other code that can still result in static linking with avrt.dll, which we should update as well, otherwise the crash could just move to these other delay-loading call sites:

• libcubeb/src/cubeb_wasapi.cpp (AvSetMmThreadCharacteristicsA, AvRevertMmThreadCharacteristics);
• libwebrtc/modules/audio_device/win/core_audio_utility_win.h (AvSetMmThreadCharacteristicsW, AvRevertMmThreadCharacteristics) but this may well be dead code unless I'm missing something.

With the patch from bug 1885501, if I manually remove the calls in media/libcubeb/src/cubeb_wasapi.cpp and source/third_party/rust/cubeb-sys/libcubeb/src/cubeb_wasapi.cpp that is enough to remove the static dependency on avrt.dll in a custom build:

# With calls
PS> dumpbin /IMPORTS C:\mozilla-source\mozilla-unified\obj-x86_64-pc-windows-msvc\dist\bin\xul.dll | Select-String avrt

    AVRT.dll

# Without calls
PS> dumpbin /IMPORTS C:\mozilla-source\mozilla-unified\obj-x86_64-pc-windows-msvc\dist\bin\xul.dll | Select-String avrt

[:glandium], [:sergesanspaille], where would be a good location to add a test to check that xul.dll does not import avrt.dll statically? Do we already have these kind of tests?

Comment 5

[Paul Adenot (:padenot)]

source/third_party/rust/cubeb-sys/libcubeb/src/cubeb_wasapi.cpp isn't compiled. We can revert to using those calls via function ptrs, it's what we used to do.

Comment 6

[Paul Adenot (:padenot)]

https://github.com/mozilla/cubeb/pull/781

Comment 7

[[:sergesanspaille]]

libxul linking is done in toolkit/library/moz.build, but we don't have a test directory there, but we may want to have one there?

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1884214 - Update libcubeb to revision aaffe75. r?#cubeb-reviewers

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1884214 - Remove patch that has been upstream. r?#cubeb-reviewers

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 1884214 - Regenerate bitrot patch and reapply the two patches. r?#cubeb-reviewers

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:padenot, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 12

[Paul Adenot (:padenot)]

I believe kinetik is going to land those.