Bug 211100 - [macOS, MacCatalyst] Dynamic access to accessibility services is incomplete
Summary: [macOS, MacCatalyst] Dynamic access to accessibility services is incomplete
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-27 15:40 PDT by Brent Fulgham
Modified: 2020-04-27 17:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.88 KB, patch)
2020-04-27 15:47 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (12.78 KB, patch)
2020-04-27 16:41 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 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'.
Comment 1 Brent Fulgham 2020-04-27 15:41:05 PDT
<rdar://problem/62133491>
Comment 2 Brent Fulgham 2020-04-27 15:47:21 PDT
Created attachment 397753 [details]
Patch
Comment 3 Brent Fulgham 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.
Comment 4 chris fleizach 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)?
Comment 5 Darin Adler 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.
Comment 6 Brent Fulgham 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.
Comment 7 Brent Fulgham 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.
Comment 8 Brent Fulgham 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.
Comment 9 Brent Fulgham 2020-04-27 16:41:41 PDT
Created attachment 397763 [details]
Patch for landing
Comment 10 chris fleizach 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
Comment 11 Per Arne Vollan 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?
Comment 12 EWS 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].
Comment 13 Brent Fulgham 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.
Comment 14 Per Arne Vollan 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?
Comment 15 Brent Fulgham 2020-04-27 17:30:09 PDT
Committed r260801: <https://trac.webkit.org/changeset/260801>