Bug 203420 – Use CF prefs direct mode in the WebContent process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Enable CF prefs direct mode, in order to avoid connecting to the CF prefs daemon in the WebContent process. In direct mode, the prefs files will be accessed directly, and not through the CF prefs daemon.

[Per Arne Vollan]

Created attachment 381935 [details]
Patch

[Geoffrey Garen]

I'd suggest doing a PLT5 A/B test on this before landing; in the past, loading a CF pref .plist into each WebContent process was a significant slowdown to process startup.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #2)
> I'd suggest doing a PLT5 A/B test on this before landing; in the past,
> loading a CF pref .plist into each WebContent process was a significant
> slowdown to process startup.

That's a good point! Perhaps we also should run a memory benchmark?

[Geoffrey Garen]

(In reply to Per Arne Vollan from comment #3)
> (In reply to Geoffrey Garen from comment #2)
> > I'd suggest doing a PLT5 A/B test on this before landing; in the past,
> > loading a CF pref .plist into each WebContent process was a significant
> > slowdown to process startup.
> 
> That's a good point! Perhaps we also should run a memory benchmark?

I think it's OK to rely on our standard bots for memory.

Two more tests you should try, though:

OpenSource/PerformanceTests/LaunchTime/new_tab.py
OpenSource/PerformanceTests/LaunchTime/startup.py

These specifically isolate process startup time. 

(You may need to enable osascript in Terminal to run these, since it is disabled by default now.)

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #4)
> (In reply to Per Arne Vollan from comment #3)
> > (In reply to Geoffrey Garen from comment #2)
> > > I'd suggest doing a PLT5 A/B test on this before landing; in the past,
> > > loading a CF pref .plist into each WebContent process was a significant
> > > slowdown to process startup.
> > 
> > That's a good point! Perhaps we also should run a memory benchmark?
> 
> I think it's OK to rely on our standard bots for memory.
> 
> Two more tests you should try, though:
> 
> OpenSource/PerformanceTests/LaunchTime/new_tab.py
> OpenSource/PerformanceTests/LaunchTime/startup.py
> 
> These specifically isolate process startup time. 
> 
> (You may need to enable osascript in Terminal to run these, since it is
> disabled by default now.)

 Sounds good! I will perform these tests :)

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #4)
> (In reply to Per Arne Vollan from comment #3)
> > (In reply to Geoffrey Garen from comment #2)
> > > I'd suggest doing a PLT5 A/B test on this before landing; in the past,
> > > loading a CF pref .plist into each WebContent process was a significant
> > > slowdown to process startup.
> > 
> > That's a good point! Perhaps we also should run a memory benchmark?
> 
> I think it's OK to rely on our standard bots for memory.
> 
> Two more tests you should try, though:
> 
> OpenSource/PerformanceTests/LaunchTime/new_tab.py
> OpenSource/PerformanceTests/LaunchTime/startup.py
> 
> These specifically isolate process startup time. 
> 
> (You may need to enable osascript in Terminal to run these, since it is
> disabled by default now.)

This patch appears to be PLT neutral on iOS and macOS. I will test launch time next.

[Per Arne Vollan]

Created attachment 388788 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #7)
> Created attachment 388788 [details]
> Patch

WIP.

[Per Arne Vollan]

Created attachment 388794 [details]
Patch

[Sam Weinig]

Comment on attachment 388794 [details]
Patch

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

> Source/WTF/wtf/Platform.h:196
> +#define ENABLE_CFPREFS_DIRECT_MODE 1

This should probably be more specific, since this is presumably only for WebKit helper services and say, would not apply to WebKit 1.  Also, as this is an ENABLE, it should go in PlatformEnable.h

[Per Arne Vollan]

Created attachment 388874 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #10)
> Comment on attachment 388794 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388794&action=review
> 
> > Source/WTF/wtf/Platform.h:196
> > +#define ENABLE_CFPREFS_DIRECT_MODE 1
> 
> This should probably be more specific, since this is presumably only for
> WebKit helper services and say, would not apply to WebKit 1.  Also, as this
> is an ENABLE, it should go in PlatformEnable.h

Made define more specific, and changed to a USE macro.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 388878 [details]
Patch

[Sam Weinig]

Comment on attachment 388878 [details]
Patch

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

> Source/WTF/wtf/Platform.h:197
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS_FAMILY)
> +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1
> +#endif

Use macros should go in PlatformUse.h!

[Per Arne Vollan]

Created attachment 388992 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #14)
> Comment on attachment 388878 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388878&action=review
> 
> > Source/WTF/wtf/Platform.h:197
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS_FAMILY)
> > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1
> > +#endif
> 
> Use macros should go in PlatformUse.h!

Fixed.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 388998 [details]
Patch

[Per Arne Vollan]

Created attachment 389136 [details]
Patch

[Geoffrey Garen]

Comment on attachment 389136 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:858
> +void WebProcess::notifyPreferencesChanged(const String& domain)
> +{
> +    _CFPreferencesFlushCachesForIdentifier(domain.createCFString().get(), kCFPreferencesCurrentUser);
> +}

Is this approach sufficient? My understanding is that, after a preference changes, the change is not written to disk immediately. Therefore, flushing the cache may trigger a re-read from disk, but the re-read may not see the new value. (I thought we were going to address this by setting the new preference explicitly in the WebContent process. Did our plan change?)

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #19)
> Comment on attachment 389136 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389136&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:858
> > +void WebProcess::notifyPreferencesChanged(const String& domain)
> > +{
> > +    _CFPreferencesFlushCachesForIdentifier(domain.createCFString().get(), kCFPreferencesCurrentUser);
> > +}
> 
> Is this approach sufficient? My understanding is that, after a preference
> changes, the change is not written to disk immediately. Therefore, flushing
> the cache may trigger a re-read from disk, but the re-read may not see the
> new value. (I thought we were going to address this by setting the new
> preference explicitly in the WebContent process. Did our plan change?)

That is a good point. I thought this would be sufficient, but I think you are right in that the new preference value might not be on disk yet. I will update the patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 390179 [details]
Patch

[Darin Adler]

Comment on attachment 390179 [details]
Patch

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

I see multiple problems here, but I don’t know the entire background.

> Source/WTF/wtf/PlatformUse.h:313
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1
> +#endif

This seems unnecessary. It’s true on all Cocoa platforms, and the only place it’s used is in a Cocoa-only source file.

> Source/WebCore/testing/Internals.cpp:5464
> +long Internals::readPreferenceInteger(const String& domain, const String& key)

long in IDL is the same as "int" in .h/.cpp so this should be int, not long

> Source/WebCore/testing/Internals.h:936
> +    long readPreferenceInteger(const String& domain, const String& key);

long in IDL is the same as "int" in .h/.cpp so this should be int, not long

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:39
> +extern "C" void _CFPrefsSetDirectModeEnabled(BOOL enabled);

This should be in an SPI header, rather than right here.

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:121
> +#if USE(CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS)

This conditional seems unnecessary. This is a Cocoa-only source file.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:28
> +@interface NSObject (PrivateKVOMethods)

This should be in an SPI header instead of here.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:37
> +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues;

I don’t think this needs to be declared. It’s part of the implementation, not the interface.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:39
> +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey, id> *)change context:(void *)context;

Ditto. Overridding something in the inherited class isn’t normally needed.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:44
> +@implementation WKUserDefaults

Why is the class implementation in a header file? I don’t think it should be.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:46
> +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues

Is this a good technique, allocating a subclass of NSUserDefaults? Is it guaranteed to work?

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:70
> +        for (auto* processPool : WebKit::WebProcessPool::allProcessPools())

Each process pool is going to have its own WKPreferenceObserver. Each WKPreferenceObserver is going to have its own sets of many WKUserDefaults objects. But all of them are going to call out to all process pools. Does this mean we are getting multiple notifyPreferencesChanged calls? I don’t think this is right.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:71
> +            processPool->notifyPreferencesChanged(m_suiteName, String(key), String(encodedString));

Should not need to explicitly write String() here.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:95
> +@implementation WKPreferenceObserver

Why is the class implementation in a header file? I don’t think it should be.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190
> +        [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil];

What is this about? Why are we observing a single test key in each defaults object? This is the kind of non-obvious code that needs comments explaining why it works.

The technique here is very difficult to understand and I don’t see sufficient test coverage, just one test.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:561
> +#if PLATFORM(COCOA)

Should not need #if PLATFORM(COCOA) in a file named ...Cocoa.mm

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:741
> +        process->send(Messages::WebProcess::NotifyPreferencesChanged(domain, key, encodedValue), 0);
> +
> +}

Extra blank line here.

[Per Arne Vollan]

(In reply to Darin Adler from comment #22)
> Comment on attachment 390179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390179&action=review
> 
> I see multiple problems here, but I don’t know the entire background.
> 
> > Source/WTF/wtf/PlatformUse.h:313
> > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1
> > +#endif
> 
> This seems unnecessary. It’s true on all Cocoa platforms, and the only place
> it’s used is in a Cocoa-only source file.
> 
> > Source/WebCore/testing/Internals.cpp:5464
> > +long Internals::readPreferenceInteger(const String& domain, const String& key)
> 
> long in IDL is the same as "int" in .h/.cpp so this should be int, not long
> 
> > Source/WebCore/testing/Internals.h:936
> > +    long readPreferenceInteger(const String& domain, const String& key);
> 
> long in IDL is the same as "int" in .h/.cpp so this should be int, not long
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:39
> > +extern "C" void _CFPrefsSetDirectModeEnabled(BOOL enabled);
> 
> This should be in an SPI header, rather than right here.
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:121
> > +#if USE(CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS)
> 
> This conditional seems unnecessary. This is a Cocoa-only source file.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:28
> > +@interface NSObject (PrivateKVOMethods)
> 
> This should be in an SPI header instead of here.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:37
> > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues;
> 
> I don’t think this needs to be declared. It’s part of the implementation,
> not the interface.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:39
> > +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey, id> *)change context:(void *)context;
> 
> Ditto. Overridding something in the inherited class isn’t normally needed.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:44
> > +@implementation WKUserDefaults
> 
> Why is the class implementation in a header file? I don’t think it should be.
> 

I will create an implementation file, PreferenceObserver.mm.

> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:46
> > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues
> 
> Is this a good technique, allocating a subclass of NSUserDefaults? Is it
> guaranteed to work?
> 

I am not aware of any issues doing this and believe it should be safe, but I will investigate and double-check to make sure.


> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:70
> > +        for (auto* processPool : WebKit::WebProcessPool::allProcessPools())
> 
> Each process pool is going to have its own WKPreferenceObserver. Each
> WKPreferenceObserver is going to have its own sets of many WKUserDefaults
> objects. But all of them are going to call out to all process pools. Does
> this mean we are getting multiple notifyPreferencesChanged calls? I don’t
> think this is right.
> 

Ah, good point! I think I will create a static NeverDestroyed<PreferenceObserver>.

> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:71
> > +            processPool->notifyPreferencesChanged(m_suiteName, String(key), String(encodedString));
> 
> Should not need to explicitly write String() here.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:95
> > +@implementation WKPreferenceObserver
> 
> Why is the class implementation in a header file? I don’t think it should be.
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190
> > +        [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil];
> 
> What is this about? Why are we observing a single test key in each defaults
> object? This is the kind of non-obvious code that needs comments explaining
> why it works.
> 
> The technique here is very difficult to understand and I don’t see
> sufficient test coverage, just one test.
> 

This is to make the preference daemon become aware of our NSUserDefaults instances, so that we get the KVO notifications. I will add a comment in the code.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:561
> > +#if PLATFORM(COCOA)
> 
> Should not need #if PLATFORM(COCOA) in a file named ...Cocoa.mm
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:741
> > +        process->send(Messages::WebProcess::NotifyPreferencesChanged(domain, key, encodedValue), 0);
> > +
> > +}
> 
> Extra blank line here.

Thanks for reviewing! I will update the patch accordingly.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #23)
> (In reply to Darin Adler from comment #22)
> > Comment on attachment 390179 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=390179&action=review
> > 
> > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190
> > > +        [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil];
> > 
> > What is this about? Why are we observing a single test key in each defaults
> > object? This is the kind of non-obvious code that needs comments explaining
> > why it works.
> > 
> > The technique here is very difficult to understand and I don’t see
> > sufficient test coverage, just one test.
> > 
> 
> This is to make the preference daemon become aware of our NSUserDefaults
> instances, so that we get the KVO notifications. I will add a comment in the
> code.
> 

We cannot use normal KVO techniques here, since we are looking for _any_ changes in a preference domain. For normal KVO techniques to work, we need to provide the specific key(s) we want to observe, but that set of keys is unknown to us.

[Per Arne Vollan]

Created attachment 390589 [details]
Patch

[Per Arne Vollan]

Created attachment 390593 [details]
Patch

[Per Arne Vollan]

Created attachment 390601 [details]
Patch

[Brent Fulgham]

<rdar://problem/58493414>

[Per Arne Vollan]

I have tested on iOS and macOS now, and have not encountered any issues with the notification mechanism so far.

[Brent Fulgham]

Comment on attachment 390601 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:90
> +    std::initializer_list<NSString*> domains = {

Do we have a way to identify other domains we will need to access if some are missing from this list?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #30)
> Comment on attachment 390601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390601&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:90
> > +    std::initializer_list<NSString*> domains = {
> 
> Do we have a way to identify other domains we will need to access if some
> are missing from this list?

Yes, I believe that whenever we need to expand the WebContent sandbox with read privileges to a new preference domain, we also need to add the domain to this list.

Thanks for reviewing!

[Per Arne Vollan]

Committed r256756: <https://trac.webkit.org/changeset/256756/webkit>