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.
Created attachment 393882 [details] Patch
Created attachment 393884 [details] Patch
Created attachment 393888 [details] Patch
Created attachment 393909 [details] Patch
rdar://problem/60542149
Created attachment 393993 [details] Patch
Created attachment 393995 [details] Patch
Created attachment 394011 [details] Patch
Created attachment 394099 [details] Patch
Created attachment 394102 [details] Patch
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.
Created attachment 394134 [details] Patch
(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!
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.
Created attachment 394142 [details] Patch
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.
(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!
Created attachment 394147 [details] Patch
(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!
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.
Created attachment 394261 [details] Patch
(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!
Created attachment 394378 [details] Patch
Committed r258949: <https://trac.webkit.org/changeset/258949> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394378 [details].
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?
(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!