Summary: | [macOS] Initialize HIToolbox library before entering sandbox | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||
Severity: | Normal | CC: | bfulgham, darin, pvollan, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2021-05-06 16:32:23 PDT
Created attachment 427956 [details]
Patch
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? Created attachment 427967 [details]
Patch
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. Created attachment 428054 [details]
Patch
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 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. The HIToolbox team suggested we use 'HiliteMenu(0)' instead, so I'll propose a patch for that. 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. Created attachment 428422 [details]
Patch
Created attachment 428425 [details]
Patch
I'm also running an A/B test to see if this triggers any PLT regression. 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. (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. 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. 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. |