Bug 209244 – [Cocoa] Fix launch time regression with CF prefs direct mode enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

When CF prefs direct mode was enabled in https://trac.webkit.org/changeset/258064/webkit, it introduced a significant launch time regression. This regression should be fixed.

[Per Arne Vollan]

Created attachment 393882 [details]
Patch

[Per Arne Vollan]

Created attachment 393884 [details]
Patch

[Per Arne Vollan]

Created attachment 393888 [details]
Patch

[Per Arne Vollan]

Created attachment 393909 [details]
Patch

[Per Arne Vollan]

rdar://problem/60542149

[Per Arne Vollan]

Created attachment 393993 [details]
Patch

[Per Arne Vollan]

Created attachment 393995 [details]
Patch

[Per Arne Vollan]

Created attachment 394011 [details]
Patch

[Per Arne Vollan]

Created attachment 394099 [details]
Patch

[Per Arne Vollan]

Created attachment 394102 [details]
Patch

[Sam Weinig]

Comment on attachment 394102 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:878
> +#if PLATFORM(COCOA)
> +#define ENABLE_CFPREFS_DIRECT_MODE 1
> +#else
>  #define ENABLE_CFPREFS_DIRECT_MODE 0
> +#endif

Please put this in PlatformEnableCocoa.h, and add a default value of ENABLE_CFPREFS_DIRECT_MODE above.

> Source/WebKit/ChangeLog:16
> +

Why is ok to wait for ui process activation? It seems like this will have a change in behavior. 

If this change is ok, please add a test showing the change in behavior.

[Per Arne Vollan]

Created attachment 394134 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #11)
> Comment on attachment 394102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394102&action=review
> 
> > Source/WTF/wtf/PlatformEnable.h:878
> > +#if PLATFORM(COCOA)
> > +#define ENABLE_CFPREFS_DIRECT_MODE 1
> > +#else
> >  #define ENABLE_CFPREFS_DIRECT_MODE 0
> > +#endif
> 
> Please put this in PlatformEnableCocoa.h, and add a default value of
> ENABLE_CFPREFS_DIRECT_MODE above.
> 

Fixed.

> > Source/WebKit/ChangeLog:16
> > +
> 
> Why is ok to wait for ui process activation? It seems like this will have a
> change in behavior. 
> 
> If this change is ok, please add a test showing the change in behavior.

In general, a user would have to deactivate Safari to be able to change preferences we are observing, so I think waiting for UI process activation should be ok. I have added a test for this.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 394134 [details]
Patch

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:-537
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.

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

How can we determine if we have registered the correct set of domains, and keep this list updated over time? Do we have to notice bugs due to incorrect behavior or is there a system for detecting mistakes in the list?

Is this a scalable approach? Will we run into performance problems again later if we have to add more domains? Created by someone else not thinking about WebKit when they start fetching more preferences in some component used by WebKit?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:681
> +#if ENABLE(CFPREFS_DIRECT_MODE)
> +    m_activationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:@"UIApplicationDidBecomeActiveNotification" object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
> +        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> +            // Start observing preference changes.
> +            [WKPreferenceObserver sharedInstance];
> +        });
> +    }];
> +#endif

Can we cut down on the copied and pasted code somehow? Maybe a helper function named startObservingPreferenceChanges?

Is there a race condition for preferences that we fetch before we call this block and create a WKPreferenceObserver, where we can miss an early change?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:-680
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.

[Per Arne Vollan]

Created attachment 394142 [details]
Patch

[Sam Weinig]

Comment on attachment 394134 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:876
> +#if !defined(ENABLE_CFPREFS_DIRECT_MODE)
>  #define ENABLE_CFPREFS_DIRECT_MODE 0
> +#endif

This should really go in the section above with all the other defaults, not down here in the "Asserts, invariants for macro definitions" section.

[Per Arne Vollan]

(In reply to Darin Adler from comment #14)
> Comment on attachment 394134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394134&action=review
> 
> > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:-537
> > -    (global-name "com.apple.cfprefsd.daemon")
> 
> Seems strange that this is not inside a conditional.
> 

AFAIK, we are not preprocessing the sandbox on iOS, so I don't believe we can use a conditional here.

> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:87
> >      std::initializer_list<NSString*> domains = {
> 
> How can we determine if we have registered the correct set of domains, and
> keep this list updated over time? Do we have to notice bugs due to incorrect
> behavior or is there a system for detecting mistakes in the list?
> 

We have done, and are doing a test sweep now to determine the initial list of domains we need. New preference domains are denied in the sandbox by default, so when we are asked to add them to the sandbox, we should also evaluate if that domain needs to go into this list. So I don't think we need to rely on noticing incorrect behavior going forward, once we have the initial list.

> Is this a scalable approach? Will we run into performance problems again
> later if we have to add more domains? Created by someone else not thinking
> about WebKit when they start fetching more preferences in some component
> used by WebKit?
> 

Given that this patch shows 0% regression on the liftoff benchmark, and that the initialization is running on a secondary thread after most other CF prefs work on startup is done, I believe we can add some domains without creating a new regression. However, I don't think we can add a lot of domains without seeing some sort of regression, either in power/memory usage or launch time. At the moment, I am not sure how many we can add before we see a regression, but I don't expect a substantial net growth in this list over time.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:681
> > +#if ENABLE(CFPREFS_DIRECT_MODE)
> > +    m_activationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:@"UIApplicationDidBecomeActiveNotification" object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
> > +        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> > +            // Start observing preference changes.
> > +            [WKPreferenceObserver sharedInstance];
> > +        });
> > +    }];
> > +#endif
> 
> Can we cut down on the copied and pasted code somehow? Maybe a helper
> function named startObservingPreferenceChanges?
> 

Fixed.

> Is there a race condition for preferences that we fetch before we call this
> block and create a WKPreferenceObserver, where we can miss an early change?
> 

That is a very good point. I think it could be possible in some scenarios, but unlikely, since the user will have to deactivate Safari before going into Settings and make changes.

> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:-680
> > -    (global-name "com.apple.cfprefsd.daemon")
> 
> Seems strange that this is not inside a conditional.

On macOS we are actually preprocessing the sandbox file, but I have observed some unexpected behavior related to ENABLE macros here earlier, so I did not use a conditional.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 394147 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #16)
> Comment on attachment 394134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394134&action=review
> 
> > Source/WTF/wtf/PlatformEnable.h:876
> > +#if !defined(ENABLE_CFPREFS_DIRECT_MODE)
> >  #define ENABLE_CFPREFS_DIRECT_MODE 0
> > +#endif
> 
> This should really go in the section above with all the other defaults, not
> down here in the "Asserts, invariants for macro definitions" section.

Thanks, fixed!

[Geoffrey Garen]

Comment on attachment 394147 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:606
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

You might as well put a dispatch_once here, so we don't do a throw-away dispatch_async on every activation.

[Per Arne Vollan]

Created attachment 394261 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #20)
> Comment on attachment 394147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394147&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:606
> > +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> 
> You might as well put a dispatch_once here, so we don't do a throw-away
> dispatch_async on every activation.

Fixed.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 394378 [details]
Patch

[EWS]

Committed r258949: <https://trac.webkit.org/changeset/258949>

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

[Chris Dumez]

Comment on attachment 394378 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> +    [[NSNotificationCenter defaultCenter] removeObserver:m_activationObserver.get()];

This introduced this crash: rdar://problem/60930466

The m_activationObserver is no longer properly unregistered on macOS... This block does not seem to be built on macOS at all.

Why did you move this?

[Per Arne Vollan]

(In reply to Chris Dumez from comment #25)
> Comment on attachment 394378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394378&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> > +    [[NSNotificationCenter defaultCenter] removeObserver:m_activationObserver.get()];
> 
> This introduced this crash: rdar://problem/60930466
> 
> The m_activationObserver is no longer properly unregistered on macOS... This
> block does not seem to be built on macOS at all.
> 
> Why did you move this?

I intended to move it so that it would be unregistered for both macOS and iOS, but did not move it to the correct place.

Thanks for catching this!