RESOLVED FIXED 211100
[macOS, MacCatalyst] Dynamic access to accessibility services is incomplete
https://bugs.webkit.org/show_bug.cgi?id=211100
Summary [macOS, MacCatalyst] Dynamic access to accessibility services is incomplete
Brent Fulgham
Reported 2020-04-27 15:40:48 PDT
Testing of MacCatalyst-based apps shows that using dynamic sandbox extensions to support accessibility features is incomplete. In addition to the 'com.apple.cfprefsd.daemon' process needed on macOS and iOS, we also need access to 'com.apple.cfprefsd.agent'.
Attachments
Patch (12.88 KB, patch)
2020-04-27 15:47 PDT, Brent Fulgham
no flags
Patch for landing (12.78 KB, patch)
2020-04-27 16:41 PDT, Brent Fulgham
no flags
Brent Fulgham
Comment 1 2020-04-27 15:41:05 PDT
Brent Fulgham
Comment 2 2020-04-27 15:47:21 PDT
Brent Fulgham
Comment 3 2020-04-27 15:47:44 PDT
I noticed some duplicate code needed when working with SandboxExtensions. I'll do that refactoring in a separate patch.
chris fleizach
Comment 4 2020-04-27 15:49:21 PDT
(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)?
Darin Adler
Comment 5 2020-04-27 15:55:25 PDT
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.
Brent Fulgham
Comment 6 2020-04-27 16:04:41 PDT
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.
Brent Fulgham
Comment 7 2020-04-27 16:05:39 PDT
(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.
Brent Fulgham
Comment 8 2020-04-27 16:41:23 PDT
> (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.
Brent Fulgham
Comment 9 2020-04-27 16:41:41 PDT
Created attachment 397763 [details] Patch for landing
chris fleizach
Comment 10 2020-04-27 16:51:16 PDT
(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
Per Arne Vollan
Comment 11 2020-04-27 17:09:37 PDT
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?
EWS
Comment 12 2020-04-27 17:12:50 PDT
Committed r260798: <https://trac.webkit.org/changeset/260798> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397763 [details].
Brent Fulgham
Comment 13 2020-04-27 17:22:50 PDT
(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.
Per Arne Vollan
Comment 14 2020-04-27 17:23:52 PDT
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?
Brent Fulgham
Comment 15 2020-04-27 17:30:09 PDT
Note You need to log in before you can comment on or make changes to this bug.