Bug 203420 - Use CF prefs direct mode in the WebContent process
Summary: Use CF prefs direct mode in the WebContent process
Status: RESOLVED FIXED
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: 212697
Blocks: 209765 210647
  Show dependency treegraph
 
Reported: 2019-10-25 09:35 PDT by Per Arne Vollan
Modified: 2020-10-01 10:59 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.81 KB, patch)
2019-10-25 09:52 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (17.23 KB, patch)
2020-01-25 15:23 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.91 KB, patch)
2020-01-25 19:04 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (29.20 KB, patch)
2020-01-27 11:06 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.98 KB, patch)
2020-01-27 11:46 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.95 KB, patch)
2020-01-28 07:42 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.92 KB, patch)
2020-01-28 08:27 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (27.89 KB, patch)
2020-01-29 07:30 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (28.25 KB, patch)
2020-02-08 13:28 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (45.86 KB, patch)
2020-02-12 16:47 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (45.90 KB, patch)
2020-02-12 17:05 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (45.95 KB, patch)
2020-02-12 17:47 PST, Per Arne Vollan
bfulgham: review+
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 2019-10-25 09:35:32 PDT
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.
Comment 1 Per Arne Vollan 2019-10-25 09:52:42 PDT
Created attachment 381935 [details]
Patch
Comment 2 Geoffrey Garen 2019-10-25 10:56:55 PDT
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.
Comment 3 Per Arne Vollan 2019-10-25 11:39:35 PDT
(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?
Comment 4 Geoffrey Garen 2019-10-25 13:54:14 PDT
(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.)
Comment 5 Per Arne Vollan 2019-10-25 14:03:25 PDT
(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 :)
Comment 6 Per Arne Vollan 2019-11-05 11:31:21 PST
(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.
Comment 7 Per Arne Vollan 2020-01-25 15:23:25 PST
Created attachment 388788 [details]
Patch
Comment 8 Per Arne Vollan 2020-01-25 15:23:53 PST
(In reply to Per Arne Vollan from comment #7)
> Created attachment 388788 [details]
> Patch

WIP.
Comment 9 Per Arne Vollan 2020-01-25 19:04:17 PST
Created attachment 388794 [details]
Patch
Comment 10 Sam Weinig 2020-01-26 10:58:22 PST
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
Comment 11 Per Arne Vollan 2020-01-27 11:06:27 PST
Created attachment 388874 [details]
Patch
Comment 12 Per Arne Vollan 2020-01-27 11:10:46 PST
(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!
Comment 13 Per Arne Vollan 2020-01-27 11:46:50 PST
Created attachment 388878 [details]
Patch
Comment 14 Sam Weinig 2020-01-27 19:54:44 PST
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!
Comment 15 Per Arne Vollan 2020-01-28 07:42:13 PST
Created attachment 388992 [details]
Patch
Comment 16 Per Arne Vollan 2020-01-28 07:42:47 PST
(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!
Comment 17 Per Arne Vollan 2020-01-28 08:27:12 PST
Created attachment 388998 [details]
Patch
Comment 18 Per Arne Vollan 2020-01-29 07:30:08 PST
Created attachment 389136 [details]
Patch
Comment 19 Geoffrey Garen 2020-01-29 11:22:17 PST
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?)
Comment 20 Per Arne Vollan 2020-01-29 11:38:32 PST
(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!
Comment 21 Per Arne Vollan 2020-02-08 13:28:41 PST
Created attachment 390179 [details]
Patch
Comment 22 Darin Adler 2020-02-09 20:58:11 PST
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.
Comment 23 Per Arne Vollan 2020-02-10 13:35:39 PST
(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.
Comment 24 Per Arne Vollan 2020-02-10 13:42:09 PST
(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.
Comment 25 Per Arne Vollan 2020-02-12 16:47:03 PST
Created attachment 390589 [details]
Patch
Comment 26 Per Arne Vollan 2020-02-12 17:05:44 PST
Created attachment 390593 [details]
Patch
Comment 27 Per Arne Vollan 2020-02-12 17:47:46 PST
Created attachment 390601 [details]
Patch
Comment 28 Brent Fulgham 2020-02-13 09:23:28 PST
<rdar://problem/58493414>
Comment 29 Per Arne Vollan 2020-02-13 13:45:50 PST
I have tested on iOS and macOS now, and have not encountered any issues with the notification mechanism so far.
Comment 30 Brent Fulgham 2020-02-16 20:12:31 PST
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?
Comment 31 Per Arne Vollan 2020-02-17 10:35:27 PST
(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!
Comment 32 Per Arne Vollan 2020-02-17 11:52:24 PST
Committed r256756: <https://trac.webkit.org/changeset/256756/webkit>