Bug 200122 - AX: web process should load correct bundle path for MACCATALYST
Summary: AX: web process should load correct bundle path for MACCATALYST
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-24 23:50 PDT by Eric Liang
Modified: 2019-08-01 16:34 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.93 KB, patch)
2019-07-25 00:02 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (1.98 KB, patch)
2019-07-25 10:07 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2019-07-25 16:58 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (2.65 KB, patch)
2019-07-25 17:07 PDT, Eric Liang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Liang 2019-07-24 23:50:27 PDT
In web process, the accessibility bundle path isn't set for MACCATALYST
Comment 1 Radar WebKit Bug Importer 2019-07-24 23:53:56 PDT
<rdar://problem/53530200>
Comment 2 Eric Liang 2019-07-25 00:02:27 PDT
Created attachment 374882 [details]
Patch
Comment 3 chris fleizach 2019-07-25 00:22:49 PDT
Comment on attachment 374882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374882&action=review

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:324
> +    NSString *accessibilityBundlePath = [(NSString *)GSSystemRootDirectory() stringByAppendingString:

this is weird to have a #define in the middle of statement. I think it should be

NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
#if PLATFORM(MACCATALYST)
accessibilityBundlePath = [accessibilityBundlePath stringByAppendingPath:@"System/iOSSupport"]
#endif
accessibilityBundlePath = [accessibilityBundlePath stringByAppendingPath:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]
Comment 4 Eric Liang 2019-07-25 10:07:21 PDT
Created attachment 374894 [details]
Patch
Comment 5 Eric Liang 2019-07-25 16:58:25 PDT
Created attachment 374924 [details]
Patch
Comment 6 chris fleizach 2019-07-25 16:59:41 PDT
Comment on attachment 374924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374924&action=review

> Source/WebKit/ChangeLog:17
> +        AX: web process should load correct bundle path for MACCATALYST

looks like we got two changelogs here

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:484
> +        "com.apple.Accessibility"

are you sure we need this one? what do we need it for?
Comment 7 Eric Liang 2019-07-25 17:05:24 PDT
Need it because in the webprocessloader bundle, we are checking whether application accessibility is enabled, which reads the pref in the iOS realm.
Comment 8 Eric Liang 2019-07-25 17:07:03 PDT
Created attachment 374925 [details]
Patch
Comment 9 chris fleizach 2019-07-25 17:08:04 PDT
(In reply to Eric Liang from comment #7)
> Need it because in the webprocessloader bundle, we are checking whether
> application accessibility is enabled, which reads the pref in the iOS realm.

Yea I can see how we probably need that for catalyst now

Brent, Per is that OK in your opinion? or should we do something else?
Comment 10 Brent Fulgham 2019-07-25 18:59:51 PDT
(In reply to chris fleizach from comment #9)
> (In reply to Eric Liang from comment #7)
> > Need it because in the webprocessloader bundle, we are checking whether
> > application accessibility is enabled, which reads the pref in the iOS realm.
> 
> Yea I can see how we probably need that for catalyst now
> 
> Brent, Per is that OK in your opinion? or should we do something else?

I need to look at this more closely. I think this is wrong, because we invested work to make it possible to dynamically add this connection when it is needed by the WebContent process.

At present, I think this patch is wrong.

Per Arne: is the dynamic AX loading something that works on macOS?
Comment 11 chris fleizach 2019-07-26 07:58:03 PDT
(In reply to Brent Fulgham from comment #10)
> (In reply to chris fleizach from comment #9)
> > (In reply to Eric Liang from comment #7)
> > > Need it because in the webprocessloader bundle, we are checking whether
> > > application accessibility is enabled, which reads the pref in the iOS realm.
> > 
> > Yea I can see how we probably need that for catalyst now
> > 
> > Brent, Per is that OK in your opinion? or should we do something else?
> 
> I need to look at this more closely. I think this is wrong, because we
> invested work to make it possible to dynamically add this connection when it
> is needed by the WebContent process.
> 
> At present, I think this patch is wrong.
> 
> Per Arne: is the dynamic AX loading something that works on macOS?

With Catalyst, we need to probably do the same stuff we're doing on iOS
Comment 12 Eric Liang 2019-07-26 09:40:30 PDT
The iOS web process had this same com.apple.accessibility pref read on its sandbox profile. On Catalyst, the web process is the iOS code but used macOS sandbox profile. This just add it to the macOS profile because it is used by the catalyst web process.
Comment 13 Brent Fulgham 2019-07-26 09:48:02 PDT
(In reply to Eric Liang from comment #12)
> The iOS web process had this same com.apple.accessibility pref read on its
> sandbox profile. On Catalyst, the web process is the iOS code but used macOS
> sandbox profile. This just add it to the macOS profile because it is used by
> the catalyst web process.

Yes -- I was having some kind of episode. I saws your pref read change and thought "xpc service connection".

The preference read is fine.
Comment 14 Brent Fulgham 2019-07-26 09:49:07 PDT
Comment on attachment 374925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374925&action=review

r=me based on Chris's review of the Accessibility bundle change, and I approve the preference read change.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:484
> +        "com.apple.Accessibility"

Yes -- this is fine. It's a preference read, not an XPC service connection as I mistakenly asserted in an early comment.
Comment 15 WebKit Commit Bot 2019-07-26 10:18:37 PDT
Comment on attachment 374925 [details]
Patch

Clearing flags on attachment: 374925

Committed r247864: <https://trac.webkit.org/changeset/247864>
Comment 16 WebKit Commit Bot 2019-07-26 10:18:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 2019-07-26 12:58:49 PDT
Comment on attachment 374925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374925&action=review

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:328
> +    NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
> +#if PLATFORM(MACCATALYST)
> +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/iOSSupport"];
> +#endif
> +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"];

Seems really unfortunate to have these paths hard-coded here in WebKit open source. Is there some way to do this better in the future? Anything in the OS that can get us the path to the accessibility bundles directory in a more principled way?
Comment 18 chris fleizach 2019-07-26 13:01:15 PDT
(In reply to Darin Adler from comment #17)
> Comment on attachment 374925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374925&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:328
> > +    NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
> > +#if PLATFORM(MACCATALYST)
> > +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/iOSSupport"];
> > +#endif
> > +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"];
> 
> Seems really unfortunate to have these paths hard-coded here in WebKit open
> source. Is there some way to do this better in the future? Anything in the
> OS that can get us the path to the accessibility bundles directory in a more
> principled way?

Yes this is doable. 

@Eric, can you stage a change in AccessibilitySupport.h that returns the path for this bundle
Comment 19 Eric Liang 2019-07-26 13:34:56 PDT
sure
Comment 20 Eric Liang 2019-08-01 16:34:32 PDT
https://bugs.webkit.org/show_bug.cgi?id=200367