Bug 211075 - [Cocoa] Global preferences are not accessible in the WebContent process when CFPrefs direct mode is enabled
Summary: [Cocoa] Global preferences are not accessible in the WebContent process when ...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-27 07:14 PDT by Per Arne Vollan
Modified: 2020-05-27 12:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.20 KB, patch)
2020-04-27 07:34 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2020-04-28 09:55 PDT, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (9.73 KB, patch)
2020-04-28 11:29 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-04-27 07:14:39 PDT
Global preferences in the domain 'kCFPreferencesAnyApplication' are not readable in the WebContent process when CFPrefs direct mode is enabled. Fix this by transferring a select set of global preferences to the WebContent process on startup.
Comment 1 Per Arne Vollan 2020-04-27 07:33:13 PDT
rdar://problem/61866711
Comment 2 Per Arne Vollan 2020-04-27 07:34:21 PDT
Created attachment 397679 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-04-27 09:06:02 PDT
Comment on attachment 397679 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Global preferences in the domain 'kCFPreferencesAnyApplication' are not readable in the WebContent process when CFPrefs direct mode
> +        is enabled. Fix this by transferring a select set of global preferences to the WebContent process on startup.

Do we have a sandbox rule already to allow reading global preference files? How about managed preferences?
Comment 4 Per Arne Vollan 2020-04-27 09:09:03 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 397679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397679&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        Global preferences in the domain 'kCFPreferencesAnyApplication' are not readable in the WebContent process when CFPrefs direct mode
> > +        is enabled. Fix this by transferring a select set of global preferences to the WebContent process on startup.
> 
> Do we have a sandbox rule already to allow reading global preference files?
> How about managed preferences?

Yes, these rules are currently present in the WebContent process' sandbox.

Thanks for reviewing!
Comment 5 Per Arne Vollan 2020-04-28 09:55:12 PDT
Created attachment 397848 [details]
Patch
Comment 6 Brent Fulgham 2020-04-28 10:19:04 PDT
Comment on attachment 397848 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481
> +    for (size_t i = 0; i < WTF_ARRAY_LENGTH(keys); ++i) {

I learned from Darin yesterday that in modern C++ (or Objective-C++) we can just do: std::size(keys).

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:489
> +        auto data = retainPtr([NSKeyedArchiver archivedDataWithRootObject:(__bridge NSMutableDictionary *)globalPreferencesDictionary.get() requiringSecureCoding:YES error:&e]);

I suggest logging if we have an error here. These can be the source of difficult problems to track down otherwise! :-)

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:182
> +    if (err)

Ditto: I'd log this error so we can find it if something is going wrong. It should be very rare.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:184
> +    for (NSString *key in globalPreferencesDictionary) {

I think you can do:

[globalPreferencesDictionary enumerateKeysAndObjectsUsingBlock: ^(NSString *key, id value, BOOL* stop) {
    if (value)
       CFPreferencesSetAppValue(static_cast<CFStringRef>(key), static_cast<CFPropertyListRef>(value), CFSTR("kCFPreferencesAnyApplication"));
}];
Comment 7 Per Arne Vollan 2020-04-28 11:29:39 PDT
Created attachment 397860 [details]
Patch
Comment 8 Per Arne Vollan 2020-04-28 11:49:58 PDT
(In reply to Brent Fulgham from comment #6)
> Comment on attachment 397848 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397848&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481
> > +    for (size_t i = 0; i < WTF_ARRAY_LENGTH(keys); ++i) {
> 
> I learned from Darin yesterday that in modern C++ (or Objective-C++) we can
> just do: std::size(keys).
> 

Fixed.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:489
> > +        auto data = retainPtr([NSKeyedArchiver archivedDataWithRootObject:(__bridge NSMutableDictionary *)globalPreferencesDictionary.get() requiringSecureCoding:YES error:&e]);
> 
> I suggest logging if we have an error here. These can be the source of
> difficult problems to track down otherwise! :-)
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:182
> > +    if (err)
> 
> Ditto: I'd log this error so we can find it if something is going wrong. It
> should be very rare.
> 

Done.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:184
> > +    for (NSString *key in globalPreferencesDictionary) {
> 
> I think you can do:
> 
> [globalPreferencesDictionary enumerateKeysAndObjectsUsingBlock: ^(NSString
> *key, id value, BOOL* stop) {
>     if (value)
>        CFPreferencesSetAppValue(static_cast<CFStringRef>(key),
> static_cast<CFPropertyListRef>(value),
> CFSTR("kCFPreferencesAnyApplication"));
> }];

That's much better.

Thanks for reviewing!
Comment 9 EWS 2020-04-28 12:29:12 PDT
Committed r260840: <https://trac.webkit.org/changeset/260840>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397860 [details].
Comment 10 Simon Fraser (smfr) 2020-04-28 13:12:01 PDT
Comment on attachment 397860 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479
> +        CFSTR("AppleLanguages"),
> +        CFSTR("AppleLanguagesSchemaVersion"),
> +        CFSTR("AppleLocale")

Do we need to respond to dynamic changes for any of these?
Comment 11 Per Arne Vollan 2020-04-28 13:20:38 PDT
(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 397860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397860&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479
> > +        CFSTR("AppleLanguages"),
> > +        CFSTR("AppleLanguagesSchemaVersion"),
> > +        CFSTR("AppleLocale")
> 
> Do we need to respond to dynamic changes for any of these?

That's already handled :)
Comment 12 Daniel Bates 2020-04-28 14:30:03 PDT
Comment on attachment 397848 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479
> +        CFSTR("AppleLocale")

Bug caught my eye because of the title. I don't know what CFPREFS_DIRECT_MODE is, but if it effects WebKit Internal debug and experimental flags toggled via Settings app then this list is not enough. Would it effect WebKitDebugVisibleDebugOverlayRegions? If it does then that would significantly effect my workflow.
Comment 13 Per Arne Vollan 2020-04-28 14:40:47 PDT
(In reply to Daniel Bates from comment #12)
> Comment on attachment 397848 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397848&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479
> > +        CFSTR("AppleLocale")
> 
> Bug caught my eye because of the title. I don't know what
> CFPREFS_DIRECT_MODE is, but if it effects WebKit Internal debug and
> experimental flags toggled via Settings app then this list is not enough.
> Would it effect WebKitDebugVisibleDebugOverlayRegions? If it does then that
> would significantly effect my workflow.

In CFPrefs direct mode, preferences are accessed directly through preference files. I don't think it would affect WebKitDebugVisibleDebugOverlayRegions, etc, but I will have to confirm.
Comment 14 Per Arne Vollan 2020-05-27 12:35:18 PDT
Reverted r260840 for reason:

This patch is no longer needed after preferences root cause has been fixed.

Committed r262207: <https://trac.webkit.org/changeset/262207>