WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Brent Fulgham
Comment 1
2020-04-27 15:41:05 PDT
<
rdar://problem/62133491
>
Brent Fulgham
Comment 2
2020-04-27 15:47:21 PDT
Created
attachment 397753
[details]
Patch
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
Committed
r260801
: <
https://trac.webkit.org/changeset/260801
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug