Bug 225494 - [macOS] Initialize HIToolbox library before entering sandbox
Summary: [macOS] Initialize HIToolbox library before entering sandbox
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-06 16:32 PDT by Brent Fulgham
Modified: 2022-02-10 16:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.79 KB, patch)
2021-05-06 16:39 PDT, Brent Fulgham
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2021-05-06 17:48 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2021-05-07 16:10 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2021-05-12 15:51 PDT, Brent Fulgham
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (3.02 KB, patch)
2021-05-12 16:02 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2021-05-06 16:32:23 PDT
AppKit calls into an older library (HLToolkit) to handle some menu operations. This library wants to know if its in a background application, and does so by communicating with LaunchServices. However, we close our connection to LaunchServices during WebContent process launch, which prevents these calls from working.

Although this failure is harmless on production systems (it fails as 'background process', which is what we want) it triggers assertions and logging on internal systems that masks useful logs and wastes power and cycles.

If we invoke HLTBIsCurrentProcessBackgroundOnly() at launch, we can complete our LaunchServices communications prior to cutting them off as we enter the sandbox. HLToolbox caches the result, so will perform properly for the rest of the WebContent process runtime.

<rdar://problem/77171527>
Comment 1 Brent Fulgham 2021-05-06 16:39:00 PDT
Created attachment 427956 [details]
Patch
Comment 2 Alexey Proskuryakov 2021-05-06 17:30:38 PDT
Comment on attachment 427956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427956&action=review

> Source/WebKit/ChangeLog:3
> +        [macOS] Initialize HLToolbox library before entering sandbox

Do we know if this will open any persistent XPC connections that we don't allow while sandboxed?
Comment 3 Brent Fulgham 2021-05-06 17:48:49 PDT
Created attachment 427967 [details]
Patch
Comment 4 Brent Fulgham 2021-05-07 00:20:53 PDT
Comment on attachment 427956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427956&action=review

>> Source/WebKit/ChangeLog:3
>> +        [macOS] Initialize HLToolbox library before entering sandbox
> 
> Do we know if this will open any persistent XPC connections that we don't allow while sandboxed?

The implementation of HLTBIsCurrentProcessBackgroundOnly is very simple. It calls a single LaunchServices command to read some process state and caches the result before returning the value. Subsequent calls do not call LaunchServices again.

I’m confirming we already hit this call before entering the sandbox before landing, but I don’t see a way to open further connections from this one call.
Comment 5 Brent Fulgham 2021-05-07 16:10:24 PDT
Created attachment 428054 [details]
Patch
Comment 6 Darin Adler 2021-05-08 16:43:34 PDT
Comment on attachment 428054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428054&action=review

> Source/WebKit/ChangeLog:18
> +        If we invoke HLTBIsCurrentProcessBackgroundOnly() at launch, we can complete our LaunchServices communications prior
> +        to cutting them off as we enter the sandbox. HLToolbox caches the result, so will perform properly for the rest of
> +        the WebContent process runtime.

This comment does not clarify that calling GetGrayRgn does this.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:689
> +    // Cache background state for HLToolBox so AppKit doesn't try to talk to LaunchServices after we enter the sandbox

WebKit style caller for a period at the end of the sentence.

Change log comment calls this HLToolbox but this comment calls it HLToolBox.

Comment does not make it clear why calling GetGrayRgn accomplishes what the comment says is needed.
Comment 7 Darin Adler 2021-05-08 16:44:50 PDT
Comment on attachment 428054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428054&action=review

> Source/WebKit/ChangeLog:17
> +        If we invoke HLTBIsCurrentProcessBackgroundOnly() at launch, we can complete our LaunchServices communications prior
> +        to cutting them off as we enter the sandbox. HLToolbox caches the result, so will perform properly for the rest of

This makes it sound like this will slow down web process creation. What we’re describing here sounds like synchronous IPC.
Comment 8 Brent Fulgham 2021-05-12 15:43:20 PDT
The HIToolbox team suggested we use 'HiliteMenu(0)' instead, so I'll propose a patch for that.
Comment 9 Brent Fulgham 2021-05-12 15:50:27 PDT
Comment on attachment 428054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428054&action=review

>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:689
>> +    // Cache background state for HLToolBox so AppKit doesn't try to talk to LaunchServices after we enter the sandbox
> 
> WebKit style caller for a period at the end of the sentence.
> 
> Change log comment calls this HLToolbox but this comment calls it HLToolBox.
> 
> Comment does not make it clear why calling GetGrayRgn accomplishes what the comment says is needed.

Will fix.
Comment 10 Brent Fulgham 2021-05-12 15:51:04 PDT
Created attachment 428422 [details]
Patch
Comment 11 Brent Fulgham 2021-05-12 16:02:11 PDT
Created attachment 428425 [details]
Patch
Comment 12 Brent Fulgham 2021-05-12 16:06:02 PDT
I'm also running an A/B test to see if this triggers any PLT regression.
Comment 13 Darin Adler 2021-05-12 17:13:36 PDT
Comment on attachment 428425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428425&action=review

> Source/WebKit/ChangeLog:18
> +        If we invoke HiliteMenu(0) at launch, we can complete our LaunchServices communications prior
> +        to cutting them off as we enter the sandbox. HLToolbox caches the result, so will perform properly for the rest of
> +        the WebContent process runtime.

I never did see your take on whether doing this additional work at this point is OK from a performance point of view.
Comment 14 Brent Fulgham 2021-05-21 10:23:10 PDT
(In reply to Darin Adler from comment #13)
> Comment on attachment 428425 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428425&action=review
> 
> > Source/WebKit/ChangeLog:18
> > +        If we invoke HiliteMenu(0) at launch, we can complete our LaunchServices communications prior
> > +        to cutting them off as we enter the sandbox. HLToolbox caches the result, so will perform properly for the rest of
> > +        the WebContent process runtime.
> 
> I never did see your take on whether doing this additional work at this
> point is OK from a performance point of view.

I've got some A/B tests running in the Perf bots to see if we catch any regression. I'll discuss with Per Arne before we land.
Comment 15 Brent Fulgham 2021-06-01 15:30:37 PDT
A/B tests completed on low-end and high-end hardware, and confirmed there was no regression.

Next I'll confirm this change doesn't introduce any new connections we didn't have prior to making this call.
Comment 16 Brent Fulgham 2021-06-01 17:00:57 PDT
It looks like this change causes new connections to be opened to distnoted, logs, and diagnosticd, which we don't need for other reasons and would not be closed before entering the sandbox without modifying other frameworks.

Since this change was being done to avoid the equivalent of a debug assertion in internal systems, it's not worth making the change. Eventually, we will remove these frameworks from the WebContent process entirely, resolving the problem permanently.