Stop initializing single-threaded COM in the content processes

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: Alex_Gaynor, Assigned: aklotz)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
mozilla67
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments)

Example stack:

    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


This is blocked by us
Essentially we don't want to initialize an STA in content, since the STA uses a hidden window and requires a message pump.

OTOH, we still need to properly configure COM, so just removing MainThreadRuntime is a non-starter.

I will probably need to modify that code to configure itself inside MTA instead... but we probably don't want the startup thread itself to be MTA.

In other words, this isn't a trivial fix and some thought needs to go into this. :-)
Summary: Stop initializing COM in the content processes → Stop initializing single-threaded COM in the content processes
Priority: -- → P3
Component: Widget: Win32 → IPC: MSCOM
Posted patch WIPSplinter Review
This is a WIP patch that moves COM init to the MTA in content processes.

This is not yet a complete solution because we still need to audit our code to determine whether there is anything in content that might attempt COM on the main thread.
Here's a try push with the above patch. Let's see if any tests break.

This patch takes care of a bunch of issues and does some cleanup:

  • We rename mscom::MainThreadRuntime to mscom::ProcessRuntime, as the latter
    is a more accurate name going forward.
  • We make ProcessRuntime aware of the Win32k Lockdown process mitigation
    policy. When Win32k is disabled, we perform process-wide COM initialization
    in the multi-threaded apartment (since we cannot create an STA window).
  • We refactor the mscom apartment region stuff to enable the Win32k lockdown
    pieces in ProcessRuntime.
  • We move some Gecko-specific stuff into MOZILLA_INTERNAL_API guards so that
    ProcessRuntime is usable outside of xul.dll (I will be needing it for the
    launcher process).
  • Another thing that might happen with the launcher process is that, under
    error conditions in the launcher, we create a ProcessRuntime object on a
    background thread for the purposes of telemetry logging, but we also allow
    the main thread to proceed to start as the browser. This could result in a
    scenario where the main thread, as the browser process, is attempting to
    instantiate its ProcessRuntime and ends up racing with the launcher process's
    telemetry thread which has its own ProcessRuntime. To account for this
    situation, we add mutual exclusion to the process-wide initialization code.
  • We clean up ProcessRuntime::InitializeSecurity by using Vector to set up
    the EXPLICIT_ACCESS entries.
  • We remove mscom::MainThreadClientInfo and replace it with a direct call to
    CoGetCallerTID
  • We revise all references to this class to use the new name.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Priority: P3 → P1
Blocks: 1460433
Blocks: 1527823
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d4b8d90cbd7
Rename mscom::MainThreadRuntime to mscom::ProcessRuntime and make it aware of Win32k lockdown and of multiple instantiations; r=Jamie
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbd7b92caef0
Backed out changeset 2d4b8d90cbd7 for Spider monkey failrues. CLOSED TREE
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13854b5d426c
Rename mscom::MainThreadRuntime to mscom::ProcessRuntime and make it aware of Win32k lockdown and of multiple instantiations; r=Jamie
Flags: needinfo?(aklotz)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.