Closed Bug 1381022 Opened 7 years ago Closed 4 years ago

Use non-UI message pump(s) in the content process

Categories

(Core :: DOM: Content Processes, enhancement, P5)

Product:
Core
Component:
DOM: Content Processes
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Alex_Gaynor, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Whiteboard: sb+, tpi:+)

Attachments

(3 obsolete files)

Currently in the content process, the main loop goes through MessagePumpForChildProcess which calls through to nsAppShell::ProcessNextNativeEvent. ProcessNextNativeEvent makes many win32k syscalls, notably NtUserPeekMessage (via PeekMessage), NtUserValidateHandleSecure (via DispatchMessageWorker), and NtUserPostMessage (via PostMessage). Per :jimm, it should be possible to use a non-UI message loop in content loop as: 17:17:35 <jimm> pretty sure that IO thread for only used in the ipdl background io thread
In discussing with :billm, he's hopeful that the work on bug 1350432 will remove the nsAppShell code from the content process's main loop, which would solve this for us.
Depends on: 1350432
I believe removing nsAppShell will also resolve the following stack: 20 - win32u!NtUserSystemParametersInfo win32u!NtUserSystemParametersInfo USER32!RealSystemParametersInfoW+0x82 UxTheme!ClassicSystemParametersInfoW+0x29 UxTheme!_InternalSystemParametersInfo+0x29 UxTheme!ThemeSystemParametersInfoW+0xcb USER32!SystemParametersInfoW+0xa7 xul!WinWakeLockListener::ResetScreenSaverTimeout+0x1b xul!WinWakeLockListener::Notify+0x71 xul!nsTimerImpl::Fire+0x414
Component: Security: Process Sandboxing → Widget: Win32
Whiteboard: sb+, tpi:+
Component: Widget: Win32 → DOM: Content Processes
I filed a separate bug for the appshell work I'm doing. I don't know if it will entirely resolve your issues. I think we might use UI message pumps elsewhere in the content process, but I could be wrong.
Depends on: 1384336
No longer depends on: 1350432
I'm not sure if this should be it's own issue, or it's really just a part of this, but we also initialize the COM system as a part of content process startup: win32u!NtUserMessageCall USER32!RealDefWindowProcWorker+0x1ba USER32!RealDefWindowProcW+0x52 UxTheme!_ThemeDefWindowProc+0x559 UxTheme!ThemeDefWindowProcW+0x11 USER32!DefWindowProcW+0x1b7 combase!OleMainThreadWndProc+0x25 USER32!UserCallWinProcCheckWow+0x280 USER32!DispatchClientMessage+0x9c USER32!_fnINOUTNCCALCSIZE+0x3b ntdll!KiUserCallbackDispatcherContinue win32u!NtUserCreateWindowEx+0x14 USER32!VerNtUserCreateWindowEx+0x212 USER32!CreateWindowInternal+0x1b5 USER32!CreateWindowExW+0x82 combase!InitMainThreadWnd+0x56 combase!ThreadFirstInitialize+0x136 combase!_CoInitializeEx+0x1db combase!CoInitializeEx+0x36 xul!mozilla::mscom::COMApartmentRegion<2>::{ctor}+0x11 xul!mozilla::mscom::MainThreadRuntime::MainThreadRuntime+0x6a xul!mozilla::dom::ContentProcess::{ctor}+0x36 xul!XRE_InitChildProcess+0x67c
Separate bug. Can be follow-up work once appshell lands. Please cc me.
Thanks for the quick analysis! Will file!
Blocks: 1400344
Priority: -- → P3
Depends on: 1423628
Depends on: 1426100
I believe at this point the only platform where we still pump native events in the content process is Linux. Is that right?
Blocks: 1533100

We're still initializing a UI main thread here, this results in various calls plus a bunch of COM stuff.

example stacks:

3 - win32u!NtUserQueryWindow
win32u!NtUserQueryWindow
user32!GetWindowThreadProcessId+0x83
xul!`anonymous
xul!mozilla::ipc::windows::InitUIThread+0x1b9
xul!XRE_InitChildProcess+0xa13
xul!mozilla::BootstrapImpl::XRE_InitChildProcess+0x2a
firefox!content_process_main+0xe4
firefox!NS_internal_main+0x117

3 - win32u!NtUserGetProcessWindowStation
win32u!NtUserGetProcessWindowStation
combase!CRpcResolver::GetThreadWinstaDesktop+0x6a
combase!<lambda_42f1a29483ec03d26359468a17427162>::operator()+0x85
combase!CRpcResolver::GetConnection+0x8c
combase!CoInitializeSecurity+0xb0
xul!mozilla::mscom::ProcessRuntime::InitializeSecurity+0xcfd
xul!mozilla::mscom::ProcessRuntime::InitInsideApartment+0x65
xul!mozilla::mscom::ProcessRuntime::ProcessRuntime+0x21a
xul!mozilla::mscom::ProcessRuntime::ProcessRuntime+0x24
xul!mozilla::dom::ContentProcess::ContentProcess+0x72
xul!XRE_InitChildProcess+0xb22

Assignee: nobody → jmathies
Priority: P3 → P1
Attached patch test patch (obsolete) (deleted) — — Splinter Review

This is built on top of bug 1381938. Looks like the event pump stuff in nsAppShell can just come out, although we may want to keep it around for a bit for testing.

Looks like we need some touch up work here as well -
https://searchfox.org/mozilla-central/source/ipc/mscom/ProcessRuntime.cpp#58

This code which runs really early can't detect 'mozilla.widget.disable-native-theme-for-content' though. It ends up calling CoInitializeEx which fails.

(In reply to Jim Mathies [:jimm] from comment #10)

Looks like we need some touch up work here as well -
https://searchfox.org/mozilla-central/source/ipc/mscom/ProcessRuntime.cpp#58

This code which runs really early can't detect 'mozilla.widget.disable-native-theme-for-content' though. It ends up calling CoInitializeEx which fails.

What's the problem here, exactly? That code is written to use the MTA when Win32k lockdown is turned on. Until that happens, it still uses the STA.

Attached patch test patch (obsolete) (deleted) — — Splinter Review

com init area still needs to be sorted out.

Attachment #9053696 - Attachment is obsolete: true

(In reply to Aaron Klotz [:aklotz] from comment #11)

(In reply to Jim Mathies [:jimm] from comment #10)

Looks like we need some touch up work here as well -
https://searchfox.org/mozilla-central/source/ipc/mscom/ProcessRuntime.cpp#58

This code which runs really early can't detect 'mozilla.widget.disable-native-theme-for-content' though. It ends up calling CoInitializeEx which fails.

What's the problem here, exactly? That code is written to use the MTA when
Win32k lockdown is turned on. Until that happens, it still uses the STA.

with native events turned off the init call seemed to fail. I had some other stuff turned on so I'll check it again.

Depends on: 1381938
Attached patch wip (obsolete) (deleted) — — Splinter Review
Attachment #9053761 - Attachment is obsolete: true
No longer blocks: 1533100
Depends on: 1539577
Depends on: 1539581
Attachment #9053763 - Attachment is obsolete: true

I still see this in stacks -

28 - win32u!NtUserCallOneParam
win32u!NtUserCallOneParam
xul!base::MessagePumpForUI::ProcessNextWindowsMessage
xul!mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop
xul!base::MessagePumpWin::RunWithDispatcher
xul!base::MessagePumpWin::Run
xul!MessageLoop::RunInternal
xul!MessageLoop::RunHandler

So I think we still need to touch up the type of chromium message pump we're using.

(In reply to Jim Mathies [:jimm] from comment #16)

I still see this in stacks -

28 - win32u!NtUserCallOneParam
win32u!NtUserCallOneParam
xul!base::MessagePumpForUI::ProcessNextWindowsMessage
xul!mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop
xul!base::MessagePumpWin::RunWithDispatcher
xul!base::MessagePumpWin::Run
xul!MessageLoop::RunInternal
xul!MessageLoop::RunHandler

So I think we still need to touch up the type of chromium message pump we're using.

I take it that's not on the main thread though?

Flags: needinfo?(jmathies)

(In reply to Bob Owen (:bobowen) from comment #17)

(In reply to Jim Mathies [:jimm] from comment #16)

I still see this in stacks -

28 - win32u!NtUserCallOneParam
win32u!NtUserCallOneParam
xul!base::MessagePumpForUI::ProcessNextWindowsMessage
xul!mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop
xul!base::MessagePumpWin::RunWithDispatcher
xul!base::MessagePumpWin::Run
xul!MessageLoop::RunInternal
xul!MessageLoop::RunHandler

So I think we still need to touch up the type of chromium message pump we're using.

I take it that's not on the main thread though?

No apparently not. Look like it might be one of these two -

https://searchfox.org/mozilla-central/search?q=symbol:E_%3CT_MessageLoop%3A%3AType%3E_TYPE_MOZILLA_NONMAINUITHREAD&redirect=false

which doesn't look like much fun.

Flags: needinfo?(jmathies)
Depends on: 1541186
Summary: Use a non-UI message pump in the content process → Use non-UI message pump(s) in the content process
Priority: P1 → P5

jya pointed out part of this may have been done during bug 1260828.

As far as I can tell this has now been resolved.
In the main loop we will stop pumping native events after bug 1701802 (when win32k lockdown is enabled).
Others uses should have been removed by work in bug 1260828, its only dependencies are for things in the GPU and GMP process.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: