1395259 - (win32k-webrtc) Use of win32k APIs in content process for webrtc task queue

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P3)

Description

[Alex Gaynor [:Alex_Gaynor]]

The sandboxing team is currently exploring what would be required to remove all win32k usage from the content process. WebRTC appears to be one of the consumers of win32k APIs. Here are two example stacks:

    win32u!NtUserPostThreadMessage
    USER32!PostThreadMessageW+0x32
    xul!rtc::TaskQueue::~TaskQueue+0x6e
    xul!webrtc::internal::Call::~Call+0x384
    xul!webrtc::internal::Call::`scalar
    xul!mozilla::DefaultDelete<webrtc::Call>::operator()+0x7
    xul!mozilla::UniquePtr<webrtc::Call,mozilla::DefaultDelete<webrtc::Call>
    xul!mozilla::WebRtcCallWrapper::~WebRtcCallWrapper+0x60
    xul!mozilla::WebRtcCallWrapper::`scalar
    xul!mozilla::detail::RefCounted<mozilla::WebRtcCallWrapper,1>::Release+0x36
    xul!mozilla::RefPtrTraits<mozilla::WebRtcCallWrapper>::Release+0x8
    xul!RefPtr<mozilla::WebRtcCallWrapper>::ConstRemovingRefPtrTraits<mozilla::WebRtcCallWrapper>::Release+0x8
    xul!RefPtr<mozilla::WebRtcCallWrapper>::{dtor}+0xf
    xul!mozilla::PeerConnectionMedia::~PeerConnectionMedia+0x100
    xul!mozilla::PeerConnectionMedia::`scalar
    xul!mozilla::PeerConnectionMedia::Release+0x44
    xul!mozilla::PeerConnectionMedia::SelfDestruct_m+0x97





    win32u!NtUserPostThreadMessage
    USER32!PostThreadMessageW+0x32
    xul!rtc::TaskQueue::~TaskQueue+0x6e
    xul!webrtc::AudioDeviceBuffer::~AudioDeviceBuffer+0x17d
    xul!webrtc::AudioDeviceModuleImpl::~AudioDeviceModuleImpl+0xc4
    xul!rtc::RefCountedObject<webrtc::AudioDeviceModuleImpl>::{dtor}+0xb
    xul!rtc::RefCountedObject<webrtc::AudioDeviceModuleImpl>::`scalar
    xul!rtc::RefCountedObject<webrtc::AudioDeviceModuleImpl>::Release+0x1a
    xul!rtc::scoped_refptr<webrtc::AudioDeviceModule>::operator=+0x20
    xul!rtc::scoped_refptr<webrtc::AudioDeviceModule>::operator=+0x9
    xul!webrtc::voe::SharedData::set_audio_device+0xf
    xul!webrtc::VoEBaseImpl::TerminateInternal+0x137
    xul!webrtc::VoEBaseImpl::Terminate+0x20
    xul!mozilla::MediaEngineWebRTCMicrophoneSource::DeInitEngine+0x12
    xul!mozilla::MediaEngineWebRTCMicrophoneSource::Deallocate+0x40
    xul!mozilla::MediaDevice::Deallocate+0x12
    xul!mozilla::SourceListener::StopTrack::__l2::<lambda_1cccf4733557b2b5acea39d2162e0e3d>::operator()+0x2b

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Not sure if this should be a separate bug or if it'd be easier to just include this here, but webrtc audio session initialization also invokes some win32k APIs:

    win32u!NtUserRegisterWindowMessage
    USER32!RegisterWindowMessageW+0x2c
    WINMMBASE!JoyInit+0x3c
    WINMMBASE!InitDevices+0x143
    WINMMBASE!ClientUpdatePnpInfo+0xb2
    WINMMBASE!mixerGetNumDevs+0xb
    xul!webrtc::AudioMixerManager::EnumerateAll+0x37
    xul!webrtc::AudioDeviceWindowsWave::Init+0x82
    xul!webrtc::AudioDeviceModuleImpl::Init+0x103
    xul!webrtc::VoEBaseImpl::Init+0x269
    xul!mozilla::WebrtcAudioConduit::Init+0xe0
    xul!mozilla::AudioSessionConduit::Create+0x9e
    xul!mozilla::MediaPipelineFactory::GetOrCreateAudioConduit+0x83
    xul!mozilla::MediaPipelineFactory::CreateOrUpdateMediaPipeline+0x410
    xul!mozilla::PeerConnectionMedia::UpdateMediaPipelines+0xba
    xul!mozilla::PeerConnectionImpl::SetSignalingState_m+0x126
    xul!mozilla::PeerConnectionImpl::SetLocalDescription+0x2c8
    xul!mozilla::PeerConnectionImpl::SetLocalDescription+0x7c
    xul!mozilla::dom::PeerConnectionImplBinding::setLocalDescription+0xdc

Comment 3

[Jim Mathies [:jimm]]

Hey Jesup, do you know if this is the "old device layer" of webrtc? If so I think the removal of this is being handled in bug 1394163.

Comment 4

[Jim Mathies [:jimm]]

>     win32u!NtUserRegisterWindowMessage
>     USER32!RegisterWindowMessageW+0x2c
>     WINMMBASE!JoyInit+0x3c
>     WINMMBASE!InitDevices+0x143
>     WINMMBASE!ClientUpdatePnpInfo+0xb2
>     WINMMBASE!mixerGetNumDevs+0xb
>     xul!webrtc::AudioMixerManager::EnumerateAll+0x37
>     xul!webrtc::AudioDeviceWindowsWave::Init+0x82
>     xul!webrtc::AudioDeviceModuleImpl::Init+0x103
>     xul!webrtc::VoEBaseImpl::Init+0x269
>     xul!mozilla::WebrtcAudioConduit::Init+0xe0
>     xul!mozilla::AudioSessionConduit::Create+0x9e
>     xul!mozilla::MediaPipelineFactory::GetOrCreateAudioConduit+0x83
>     xul!mozilla::MediaPipelineFactory::CreateOrUpdateMediaPipeline+0x410
>     xul!mozilla::PeerConnectionMedia::UpdateMediaPipelines+0xba
>     xul!mozilla::PeerConnectionImpl::SetSignalingState_m+0x126
>     xul!mozilla::PeerConnectionImpl::SetLocalDescription+0x2c8
>     xul!mozilla::PeerConnectionImpl::SetLocalDescription+0x7c
>     xul!mozilla::dom::PeerConnectionImplBinding::setLocalDescription+0xdc

My previous comment was about this stack, not the TaskQueue related calls.

Comment 5

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

Yes. webrtc::AudioDeviceModule/etc are the stuff we're removing use of.

Comment 6

[Nils Ohlmeier [:drno]]

Over in bug 1376873 we are working on updating the webrtc.org code base to version 64. From looking at the new code it seems to that updating it to 64 will result in the PostThreadMessage going to dis-appear.

Comment 7

[Jim Mathies [:jimm]]

Still seeing these today - typical stacks;

2571 - win32u!NtUserMsgWaitForMultipleObjectsEx
win32u!NtUserMsgWaitForMultipleObjectsEx
user32!RealMsgWaitForMultipleObjectsEx+0x1d
xul!rtc::TaskQueue::Impl::ThreadState::RunThreadMain+0x73
xul!rtc::TaskQueue::Impl::ThreadMain+0x3d
xul!rtc::PlatformThread::Run+0x165
xul!rtc::PlatformThread::StartThread+0x24
KERNEL32!BaseThreadInitThunk+0x14
mozglue!mozilla::interceptor::FuncHook<mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyShared<mozilla::interceptor::MMPolicyInProcess,128>

237 - win32u!NtUserPostThreadMessage
win32u!NtUserPostThreadMessage
xul!PostThreadMessage+0x2e
xul!rtc::TaskQueue::Impl::PostDelayedTask+0x11e
xul!rtc::TaskQueue::PostDelayedTask+0x6b
xul!webrtc::internal::VideoSendStreamImpl::CheckEncoderActivityTask::Run+0x1f3
xul!rtc::`anonymous
xul!rtc::TaskQueue::Impl::ThreadState::RunDueTasks+0x11b

104 - win32u!NtUserPostThreadMessage
win32u!NtUserPostThreadMessage
xul!PostThreadMessage+0x2e
xul!rtc::TaskQueue::Impl::PostDelayedTask+0x11e
xul!rtc::TaskQueue::PostDelayedTask+0x6b
xul!webrtc::OveruseFrameDetector::CheckOveruseTask::Run+0x153
xul!rtc::`anonymous
xul!rtc::TaskQueue::Impl::ThreadState::RunDueTasks+0x11b

Comment 8

[Jim Mathies [:jimm]]

https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/rtc_base/task_queue_win.cc

Windows task queue implementation. This is using a background thread, thread messages and PeekMessage to process events. I guess we'll have to re-implement this somehow to avoid these calls.

Comment 9

[Jim Mathies [:jimm]]

Hey Dan, curious, Nils mentioned the webrtc 64 lib might be win32k.sys lockdown compat, but we have calls happening in this task queue code that would be blocked. Curious if you might have any background info on the webrtc roadmap related to this?

Comment 10

[Dan Minor [:dminor]]

The task queue code is still present in tip of webrtc.org, so yes I guess we'll have to reimplement their task queue if we want to get rid of these calls.

We no longer use their AudioDevice implementations.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

The Windows task queue stuff can be implemented in terms of APCs and waitable timers.

Each thread has an implicit queue for user-mode APCs. Use QueueUserAPC to post them, and one of the alertable wait functions to block while waiting for an APC to fire.

Waitable timers integrate with the wait functions by signaling a HANDLE and optionally by posting an APC.

The quit message could be implemented in a couple of ways: You could either use an event object that ProcessQueuedMessages waits on via WaitForSingleObjectEx or WaitForMultipleObjectsEx, or you could just enqueue a special APC that signals that it is time for the thread to break out of its event loop. Which one you choose probably depends on whether or not you want the event queue to be drained prior to quitting.

Comment 12

[Nils Ohlmeier [:drno]]

So it turns out that Chrome is actually not using the TaskQueue, because it gets over written at link time with their own user space task queue implementation. It is not clear thought what that would mean for us re-implementing these TaskQueue code upstream.

Comment 13

[June Wilde (she/her) [:jewilde]]

Attachment: Bug 1395259 - Remove win32k usage from webrtc's TaskQueue; r=dminor

Comment 14

[June Wilde (she/her) [:jewilde]]

Here's a quick run through of all of the changes I've made so far and my current status. Sorry for this being a tad lengthy and also sorry for the noise made by mach clang-format in the patch.

Working through Aaron's suggestions (thank you again for these!) I found that TaskQueue already uses a single user-mode APC through WorkerThread::QueueAPC to post all of its tasks to. Also, I went with the option to have an event object for TaskQueue shutdown and to add that event to the handles we wait on in RunThreadMain. This is so we can clear the queues of any tasks we have left. The timer events have also been replaced by Waitable Timers.

The ThreadState class has been removed to simplify the TaskQueue::Impl class and to remove the need to duplicate the handles between the two instances and to move closer to matching the TaskQueue implementations on Linux and macOS.

TaskQueue::Impl::ThreadState::ProcessQueuedMessages was merged into TaskQueue::Impl::RunThreadMain since most of ProcessQueuedMessages' structure had been removed and RunThreadMain was the only caller and so that we could use the same WaitForMultipleObjectsEx call to wait on all of the necessary event handles.

ThreadState::timer_id_ was renamed to task_timer_ to better differentiate from MultimediaTimer::timer_id_ and changed from a UINT_PTR to a HANDLE since it now tracks an Waitable Timer event.

TaskQueue::Impl::stop_queue_ was added to track an event meant to signal shut down of the TaskQueue thread. TaskQueue::Impl destructor now cancels any remaining timers left open as well. TaskQueue::Impl::RunPendingTasks now resets the in_queue_ event if the queue of tasks is empty.

TaskQueue::Impl::RunThreadMain now waits on an event for thread shutdown, an event for delayed tasks, an event for media tasks, and an event for processing due tasks. kMaxTaskProcessingTime was removed but might need to be put back in as it's been a while since I merged ProcessQueuedMessages and RunThreadMain so that might have just been negligently removed.

Win32K calls removed:

• Line 56 call to ::PeekMessage was removed since we don't need to init the window message queue anymore.

• Line 267 ::PostThreadMessage was replaced with posting a lambda to the task queue that signals the stop_queue_ event for TaskQueue shutdown.

• Line 308 ::PostThreadMessage was replaced with posting a lambda to the task queue and rescheduling the Waitable Timer based on whether or not our new task is now the next task to be run.

• Line 325 ::PostThreadMessage was replaced by just running the task used for the reply in place rather than dumping it back onto the queue of tasks. This was done to avoid having to maintain a reference to the queue inside the pre-existing lambda.

• Line 360 ::MsgWaitForMultipleObjectsEx was replaced with a call to ::WaitForMultipleObjectsEx to continue putting the thread into an alertable wait and continue listening for our events without the need for listening to window messages.

• Line 417 ::KillTimer was replaced with ::CancelWaitableTimer.

• Line 459 ::SetTimer was replaced with ::SetWaitableTimer.

• Line 465 ::KillTimer was replaced with ::CancelWaitableTimer.

TODO items handled:

• Custom comparison function for the priority_queue was replaced with std::greater since we build with C++14 now. (need to double check that this is okay upstream)

• Line 303 the pointer to the DelayedTaskInfo object is needed here now for more than just being placed into a thread message, so the extra allocation should be fine.

• Line 395 we no longer use window messaging to queue tasks, which I believe this comment was referencing.

Comment 15

[June Wilde (she/her) [:jewilde]]

Most of the webrtc samples are working with this patch and the remaining failures are in a handful of tests, although I can't reproduce most of them locally and have had a bit of trouble tracking down what is causing the failures in what I can reproduce.

wpt4 & wpt10 & wpt16

• process crash in webrtc/protocol/ice-state.https.html
• process crash in webrtc/RTCPeerConnection-getStats.https.html
• process crash in css/css-transforms/animation/translate-interpolation.html

mda2 & mda3 & Mochitest with fission mda

• process crash in dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsolation.html

Mochitest with fission chunk 3

• test fail in dom/serviceworkers/test/test_https_origin_after_redirect.html
• test fail in dom/security/test/mixedcontentblocker/test_main.html

I've been updating the attached phabricator patch with my latest edits and here is my latest try push.

Comment 16

[June Wilde (she/her) [:jewilde]]

Current try push with all of the Win32k API calls removed is finally green!

Pending reviews I'll attach a copy of this based on upstream and we can attempt to get this tested in and pushed to upstream as well. I'll ni Nils when that happens so that we can decide how best to get these changes in-tree.

Comment 17

[June Wilde (she/her) [:jewilde]]

Nils: How would you like us to land this on the in-tree copy of the WebRTC library?

Comment 18

[June Wilde (she/her) [:jewilde]]

Upstreaming happening with bug here and review here

Comment 19

[Nils Ohlmeier [:drno]]

(In reply to June Wilde (she/her) [:jewilde] from comment #17)

Nils: How would you like us to land this on the in-tree copy of the WebRTC library?

Yes. Let's land this in mozilla-central.

And then we can take care of upstreaming it separately, which it looks like you started already. That is awesome!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0be56d0dd63
Remove win32k usage from webrtc's TaskQueue; r=dminor

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c0be56d0dd63