1467758 - WindowServer connection never being terminated

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P2)

Description

[Alex Plaskett]

The plugin-container content processes will never terminate the connection to the WindowServer using CGSSetDenyWindowServerConnections and CGSShutdownServerConnections. 

XRE_UseNativeEventProcessing will always return true as the default preference dom.ipc.useNativeEventProcessing.content is set to true by default on macOS (https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#3218).

We can also confirm this under lldb, attached to a plugin content process by printing the value of deny_future_windowserver_connections, which will always be 0. 

It is recommended this is reviewed.  

Annotated code as follows:

https://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1612

static bool
StartMacOSContentSandbox()
{
  int sandboxLevel = GetEffectiveContentSandboxLevel();
  if (sandboxLevel < 1) {
    return false;
  }

  if (!XRE_UseNativeEventProcessing()) {
    // If we've opened a connection to the window server, shut it down now. Forbid
    // future connections as well. We do this for sandboxing, but it also ensures
    // that the Activity Monitor will not label the content process as "Not
    // responding" because it's not running a native event loop. See bug 1384336.
    CGSSetDenyWindowServerConnections(true);
    CGSShutdownServerConnections();
  }

bool XRE_UseNativeEventProcessing()
{
  if (XRE_IsContentProcess()) {                                                                               // In the case of the content process
    static bool sInited = false;
    static bool sUseNativeEventProcessing = false;
    if (!sInited) {
      Preferences::AddBoolVarCache(&sUseNativeEventProcessing,
                                   "dom.ipc.useNativeEventProcessing.content");                               // This pulls back existing true cache value if its not already set.
      sInited = true;
    }

    return sUseNativeEventProcessing;                                                                         // Will return true here as well based on default pref value.
  }

  return true;                                                                                            
}


//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

  // Adds the aVariable to cache table. |aVariable| must be a pointer for a
  // static variable. The value will be modified when the pref value is changed
  // but note that even if you modified it, the value isn't assigned to the
  // pref.
  static nsresult AddBoolVarCache(bool* aVariable,
                                  const char* aPref,
                                  bool aDefault = false,
                                  bool aSkipAssignment = false);


 /* static */ nsresult
Preferences::AddBoolVarCache(bool* aCache,
                             const char* aPref,
                             bool aDefault,
                             bool aSkipAssignment)
{
  AssertNotAlreadyCached("bool", aPref, aCache);
  if (!aSkipAssignment) {                                               // aSkipAssignment is false by default 
    *aCache = GetBool(aPref, aDefault);                                 // GetBool retrieves true and sets sUseNativeEventProcessing to true. 
  }
  CacheData* data = new CacheData();
  data->mCacheLocation = aCache;
  data->mDefaultValueBool = aDefault;
  CacheDataAppendElement(data);
  Preferences::RegisterCallback(BoolVarChanged,
                                aPref,
                                data,
                                Preferences::ExactMatch,
                                /* isPriority */ true);
  return NS_OK;
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

To summarize the results of the research in bug 1426100, and the state of this bug:

Originally we thought the blocker to disconnecting the windowserver was removing native event processing (which needed some performance work to be landable). However, on implementing that and digging in, I found that windowserver connections were still hanging around.

Disassembly of the macOS system libraries showed that we (along with every other browser) were misusing the (undocumented, private) disconnect-windowserver APIs. Correcting our usage of them produced (a) a browser with no windowserver connections, (b) a browser where OpenGL didn't work.

Digging into the OpenGL code I found that many of these APIs require a windowserver connection. Therefore disconnecting from the windowserver will require WebGL's OpenGL interactions to be remoted.

Therefore the route to go is (a) land the removal of native event processing without touching the windowserver code (this is what the latest patch on bug 1426100 does), (b) once the WebGL remoting is completed, turn off the windowserver connection.

(Both Chromium and WebKit were notified that they had the same bug as we did with disconnecting from the windowserver.)

Comment 3

[Haik Aftandilian [:haik]]

This may also depend on non-native for Mac. On 10.14, without the WindowServer, some HTML form elements such as radio buttons are not rendered. More debugging needed.

Comment 4

[Tom Ritter [:tjr]]

:handyman, do you think you could weigh in on the WebGL bits here? Without reproducing Alex's research - what bugs would you guess are blockers for this, to give us an idea of when we can re-evaluate the situation here?

Comment 5

[Haik Aftandilian [:haik]]

For testing, setting the pref security.sandbox.content.mac.disconnect-windowserver=true (which doesn't exist by default) will change the sandbox rules to not allow the WindowServer connection.

Comment 6

[David Parks [:handyman]]

Bug tracking for this is not great ATM but some useful bugs:

Bug 1621762 - Add IpdlQueue option for remote WebGL
Current WIP that adds functionality for Linux/Mac WebGL remoting. This will land soon.

Bug 1624726 - Refactor of WebGL remoting classes
Just some code cleanup.

Bug 1607940 - WebGL out-of-process prototype
The bug for landing webgl remoting turned on. ATM, this is only planned for Windows as there may well be bad performance regressions on other platforms (but that is not certain). If regressions aren't too bad then we will use this to enable all platforms. There are a few pieces of this that are still somewhat unsettled (I think just related to textures and VR) but otherwise the plan for this is concrete. We can make this the dependency for now.

Comment 7

[David Parks [:handyman]]

I should clarify that bug 1621762 adds some behavior needed for Linux and Mac but won't be a complete working implementation that people can play with.

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Bug 1467758 - WindowServer connection never being terminated r?spohl

Drop the window server connection from the content process sandbox when out-of-process WebGL is enabled.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5aaf8123b97a
WindowServer connection never being terminated r=spohl

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5aaf8123b97a

Comment 11

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5aaf8123b97a
WindowServer connection never being terminated r=spohl

== Change summary for alert #32053 (as of Fri, 22 Oct 2021 11:58:42 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
74%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s fission stylo webrender	420.58 -> 109.33
73%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s stylo webrender	399.29 -> 108.92
72%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s stylo webrender	386.08 -> 109.92
70%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s fission stylo webrender	369.21 -> 112.00
70%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s stylo webrender-sw	385.96 -> 117.25
...	...	...	...	...
7%	perf_reftest_singletons coalesce-2.html	macosx1014-64-shippable-qr	e10s stylo webrender	205.19 -> 190.66

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=32053

Comment 12

[Haik Aftandilian [:haik]]

On a MacBook Pro 13-inch, M1, 2020, the change in cpstartup with the profiler running is around a 30% reduction (from 70ms to 48ms) from some quick measurements.

One noticeable difference in the profiles on the 2020 M1 with/without the fix is that before removing WindowServer access, the call to mozilla::ipc::SetThisProcessName() ends up in macOS SkyLight function SLSMainConnectionID() presumably getting the WindowServer connection info which takes ~20ms. This gets skipped with this change and that amounts to most of the difference on this faster machine. (Faster when compared to the CI test machines.)