Bug 211100

<rdar://problem/62133491>

Created attachment 397753 [details] Patch

I noticed some duplicate code needed when working with SandboxExtensions. I'll do that refactoring in a separate patch.

(In reply to Brent Fulgham from comment #3) > I noticed some duplicate code needed when working with SandboxExtensions. > I'll do that refactoring in a separate patch. Were you able to confirm that it does work on macOS (not just catalyst)?

Comment on attachment 397753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397753&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:461 > + static const char* services[] = { Can make this more const like this: static constexpr const char* services [] { The other way would result in less efficient code. > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:467 > + auto size = WTF_ARRAY_LENGTH(services); Don’t need WTF_ARRAY_LENGTH any more in modern C++: auto size = std::size(services); > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:254 > + SandboxExtension::HandleArray handleArray; > + static const char* services[] = { > +#if PLATFORM(MACCATALYST) > + "com.apple.cfprefsd.agent", > +#endif > + "com.apple.cfprefsd.daemon" > + }; > + auto size = WTF_ARRAY_LENGTH(services); > + handleArray.allocate(size); > + for (size_t i = 0; i < size; ++i) { > + if (!SandboxExtension::createHandleForMachLookup(services[i], connection() ? connection()->getAuditToken() : WTF::nullopt, handleArray[i])) > + return; > + } Can we share this code? Comments above still apply.

Comment on attachment 397753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397753&action=review >> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:461 >> + static const char* services[] = { > > Can make this more const like this: > > static constexpr const char* services [] { > > The other way would result in less efficient code. Sure! >> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:467 >> + auto size = WTF_ARRAY_LENGTH(services); > > Don’t need WTF_ARRAY_LENGTH any more in modern C++: > > auto size = std::size(services); Oh, great! I'll change that. >> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:254 >> + } > > Can we share this code? Comments above still apply. Maybe. I want to do a separate patch to make SandboxExtension::HandleArray more useful, but I didn't want that more wide-spread change to clutter this patch. My proposed SandboxExtension change will address some of this repetitive boilerplate. I'm less user how to capture the knowledge that these two cfprefsd connections are needed. We need to know this when launching, or here when we toggle the process. Maybe I could add a static method to WebProcessPool to give us the array of services, and use it in both places.

(In reply to chris fleizach from comment #4) > (In reply to Brent Fulgham from comment #3) > > I noticed some duplicate code needed when working with SandboxExtensions. > > I'll do that refactoring in a separate patch. > > Were you able to confirm that it does work on macOS (not just catalyst)? I'll double-check. Do our AX unit tests not cover this case? I'm building locally and will do the manual test we do for MacCatalyst to confirm the same behavior.

> (In reply to chris fleizach from comment #4) > Were you able to confirm that it does work on macOS (not just catalyst)? Testing manually, I found that I needed the 'agent' as well. I've modified the patch to match.

Created attachment 397763 [details] Patch for landing

(In reply to Brent Fulgham from comment #7) > (In reply to chris fleizach from comment #4) > > (In reply to Brent Fulgham from comment #3) > > > I noticed some duplicate code needed when working with SandboxExtensions. > > > I'll do that refactoring in a separate patch. > > > > Were you able to confirm that it does work on macOS (not just catalyst)? > > I'll double-check. Do our AX unit tests not cover this case? > > I'm building locally and will do the manual test we do for MacCatalyst to > confirm the same behavior. We read the value with _AXSIsolatedTreeMode() but there's no test to check this value right now

Comment on attachment 397763 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=397763&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:244 > + "com.apple.cfprefsd.agent", Is this needed on iOS?

Committed r260798: <https://trac.webkit.org/changeset/260798> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397763 [details].

(In reply to Per Arne Vollan from comment #11) > Comment on attachment 397763 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=397763&action=review > > > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:244 > > + "com.apple.cfprefsd.agent", > > Is this needed on iOS? We probably should -- I didn't debug on device to confirm, but since it's needed on macOS and macCatalyst it seems likely.

Comment on attachment 397763 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=397763&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:108 > +#if !PLATFORM(IOS_FAMILY) || PLATFORM(MACCATALYST) > static NSString *WebKitApplicationDidChangeAccessibilityEnhancedUserInterfaceNotification = @"NSApplicationDidChangeAccessibilityEnhancedUserInterfaceNotification"; > #endif It seems this string is unused on Catalyst, or perhaps I am mistaken?

Committed r260801: <https://trac.webkit.org/changeset/260801>