Bug 225494 – [macOS] Initialize HIToolbox library before entering sandbox

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Brent Fulgham]

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>

[Brent Fulgham]

Created attachment 427956 [details]
Patch

[Alexey Proskuryakov]

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?

[Brent Fulgham]

Created attachment 427967 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 428054 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Brent Fulgham]

The HIToolbox team suggested we use 'HiliteMenu(0)' instead, so I'll propose a patch for that.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 428422 [details]
Patch

[Brent Fulgham]

Created attachment 428425 [details]
Patch

[Brent Fulgham]

I'm also running an A/B test to see if this triggers any PLT regression.

[Darin Adler]

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.

[Brent Fulgham]

(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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.