Bug 211075 – [Cocoa] Global preferences are not accessible in the WebContent process when CFPrefs direct mode is enabled

• Status: REOPENED
• Component: WebKit Misc.

[Per Arne Vollan]

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.

[Per Arne Vollan]

rdar://problem/61866711

[Per Arne Vollan]

Created attachment 397679 [details]
Patch

[Alexey Proskuryakov]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 397848 [details]
Patch

[Brent Fulgham]

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"));
}];

[Per Arne Vollan]

Created attachment 397860 [details]
Patch

[Per Arne Vollan]

(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!

[EWS]

Committed r260840: <https://trac.webkit.org/changeset/260840>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397860 [details].

[Simon Fraser (smfr)]

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?

[Per Arne Vollan]

(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 :)

[Daniel Bates]

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.

[Per Arne Vollan]

(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.

[Per Arne Vollan]

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>