Summary: | [Cocoa] Global preferences are not accessible in the WebContent process when CFPrefs direct mode is enabled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||
Status: | REOPENED --- | ||||||||||
Severity: | Normal | CC: | bfulgham, dbates, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Per Arne Vollan
2020-04-27 07:14:39 PDT
Created attachment 397679 [details]
Patch
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? (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! Created attachment 397848 [details]
Patch
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")); }]; Created attachment 397860 [details]
Patch
(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! Committed r260840: <https://trac.webkit.org/changeset/260840> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397860 [details]. 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? (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 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 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. 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> |