Check for inAppBrowserPrivacyEnabled flag in WebsiteDataStore::initializeAppBoundDoma...
authorkatherine_cheney@apple.com <katherine_cheney@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Jun 2020 22:57:06 +0000 (22:57 +0000)
committerkatherine_cheney@apple.com <katherine_cheney@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Jun 2020 22:57:06 +0000 (22:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213261
<rdar://problem/64317084>

Reviewed by Brent Fulgham.

There are two bugs here. First, WebsiteDataStore::parameters()
does a lot of work aside from returning the parameters, making this an
expensive call just to check the value of one flag. Second,
appBoundDomains() is a static hashset, and initializing the hashset
should not rely on non-static functions.

* NetworkProcess/NetworkSessionCreationParameters.cpp:
(WebKit::NetworkSessionCreationParameters::encode const):
(WebKit::NetworkSessionCreationParameters::decode):
* NetworkProcess/NetworkSessionCreationParameters.h:
* NetworkProcess/cocoa/NetworkSessionCocoa.h:
* NetworkProcess/cocoa/NetworkSessionCocoa.mm:
(WebKit::NetworkSessionCocoa::NetworkSessionCocoa):
Remove flag, it is no longer used here.

* UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:
(WebKit::WebsiteDataStore::platformSetNetworkParameters):
Save the flag value in a member variable so we can use that later
when checking app-bound domains. Rename to something more descriptive.

(WebKit::WebsiteDataStore::initializeAppBoundDomains):
Remove the check for the non-static runtime flag.

(WebKit::WebsiteDataStore::ensureAppBoundDomains const):
Enable test mode quirks. We must do it in two places in case the
initialization hasn't happened by the time we call
ensureAppBoundDomains.

* UIProcess/WebsiteData/WebsiteDataStore.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263122 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp
Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h
Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h
Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h

index ce31b90..bca7938 100644 (file)
@@ -1,3 +1,41 @@
+2020-06-16  Kate Cheney  <katherine_cheney@apple.com>
+
+        Check for inAppBrowserPrivacyEnabled flag in WebsiteDataStore::initializeAppBoundDomains is expensive and calls a non-static function WebsiteDataStore::parameters().
+        https://bugs.webkit.org/show_bug.cgi?id=213261
+        <rdar://problem/64317084>
+
+        Reviewed by Brent Fulgham.
+
+        There are two bugs here. First, WebsiteDataStore::parameters()
+        does a lot of work aside from returning the parameters, making this an
+        expensive call just to check the value of one flag. Second,
+        appBoundDomains() is a static hashset, and initializing the hashset
+        should not rely on non-static functions.
+
+        * NetworkProcess/NetworkSessionCreationParameters.cpp:
+        (WebKit::NetworkSessionCreationParameters::encode const):
+        (WebKit::NetworkSessionCreationParameters::decode):
+        * NetworkProcess/NetworkSessionCreationParameters.h:
+        * NetworkProcess/cocoa/NetworkSessionCocoa.h:
+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+        (WebKit::NetworkSessionCocoa::NetworkSessionCocoa):
+        Remove flag, it is no longer used here.
+
+        * UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:
+        (WebKit::WebsiteDataStore::platformSetNetworkParameters):
+        Save the flag value in a member variable so we can use that later
+        when checking app-bound domains. Rename to something more descriptive.
+
+        (WebKit::WebsiteDataStore::initializeAppBoundDomains):
+        Remove the check for the non-static runtime flag.
+
+        (WebKit::WebsiteDataStore::ensureAppBoundDomains const):
+        Enable test mode quirks. We must do it in two places in case the
+        initialization hasn't happened by the time we call
+        ensureAppBoundDomains.
+
+        * UIProcess/WebsiteData/WebsiteDataStore.h:
+
 2020-06-16  Mark Lam  <mark.lam@apple.com>
 
         Make Options::useJIT() be the canonical source of truth on whether we should use the JIT.
index 901deb8..3d5180a 100644 (file)
@@ -77,7 +77,6 @@ void NetworkSessionCreationParameters::encode(IPC::Encoder& encoder) const
     encoder << testSpeedMultiplier;
     encoder << suppressesConnectionTerminationOnSystemChange;
     encoder << allowsServerPreconnect;
-    encoder << isInAppBrowserPrivacyEnabled;
     encoder << requiresSecureHTTPSProxyConnection;
     encoder << preventsSystemHTTPProxyAuthentication;
     encoder << resourceLoadStatisticsParameters;
@@ -236,11 +235,6 @@ Optional<NetworkSessionCreationParameters> NetworkSessionCreationParameters::dec
     decoder >> allowsServerPreconnect;
     if (!allowsServerPreconnect)
         return WTF::nullopt;
-    
-    Optional<bool> isInAppBrowserPrivacyEnabled;
-    decoder >> isInAppBrowserPrivacyEnabled;
-    if (!isInAppBrowserPrivacyEnabled)
-        return WTF::nullopt;
 
     Optional<bool> requiresSecureHTTPSProxyConnection;
     decoder >> requiresSecureHTTPSProxyConnection;
@@ -295,7 +289,6 @@ Optional<NetworkSessionCreationParameters> NetworkSessionCreationParameters::dec
         , WTFMove(*testSpeedMultiplier)
         , WTFMove(*suppressesConnectionTerminationOnSystemChange)
         , WTFMove(*allowsServerPreconnect)
-        , WTFMove(*isInAppBrowserPrivacyEnabled)
         , WTFMove(*requiresSecureHTTPSProxyConnection)
         , WTFMove(*preventsSystemHTTPProxyAuthentication)
         , WTFMove(*resourceLoadStatisticsParameters)
index 3adaaef..7d42aa4 100644 (file)
@@ -90,7 +90,6 @@ struct NetworkSessionCreationParameters {
     unsigned testSpeedMultiplier { 1 };
     bool suppressesConnectionTerminationOnSystemChange { false };
     bool allowsServerPreconnect { true };
-    bool isInAppBrowserPrivacyEnabled { false };
     bool requiresSecureHTTPSProxyConnection { false };
     bool preventsSystemHTTPProxyAuthentication { false };
     
index 687c67f..9ca8fa6 100644 (file)
@@ -98,7 +98,6 @@ public:
 
     SessionWrapper& sessionWrapperForTask(const WebCore::ResourceRequest&, WebCore::StoredCredentialsPolicy, Optional<NavigatingToAppBoundDomain>);
     void clearAppBoundSession() override;
-    bool isInAppBrowserPrivacyEnabled() const { return m_isInAppBrowserPrivacyEnabled; }
     bool preventsSystemHTTPProxyAuthentication() const { return m_preventsSystemHTTPProxyAuthentication; }
     
     void clientCertificateSuggestedForHost(NetworkDataTaskCocoa::TaskIdentifier, NSURLCredential *, const String& host, uint16_t port);
@@ -150,7 +149,6 @@ private:
     Seconds m_loadThrottleLatency;
     bool m_fastServerTrustEvaluationEnabled { false };
     String m_dataConnectionServiceType;
-    bool m_isInAppBrowserPrivacyEnabled { false };
     bool m_preventsSystemHTTPProxyAuthentication { false };
 
     struct SuggestedClientCertificate {
index 2e1d1ce..a2bb74d 100644 (file)
@@ -1162,7 +1162,6 @@ NetworkSessionCocoa::NetworkSessionCocoa(NetworkProcess& networkProcess, Network
     , m_loadThrottleLatency(parameters.loadThrottleLatency)
     , m_fastServerTrustEvaluationEnabled(parameters.fastServerTrustEvaluationEnabled)
     , m_dataConnectionServiceType(parameters.dataConnectionServiceType)
-    , m_isInAppBrowserPrivacyEnabled(parameters.isInAppBrowserPrivacyEnabled)
     , m_preventsSystemHTTPProxyAuthentication(parameters.preventsSystemHTTPProxyAuthentication)
 {
     ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessRawCookies));
index d629a6d..a9504d3 100644 (file)
@@ -117,6 +117,8 @@ void WebsiteDataStore::platformSetNetworkParameters(WebsiteDataStoreParameters&
         else
             firstPartyWebsiteDataRemovalMode = WebCore::FirstPartyWebsiteDataRemovalMode::AllButCookies;
     }
+    m_isInAppBrowserPrivacyTestModeEnabled = [defaults boolForKey:[NSString stringWithFormat:@"WebKitDebug%@", WebPreferencesKey::isInAppBrowserPrivacyEnabledKey().createCFString().get()]];
+
     auto* manualPrevalentResource = [defaults stringForKey:@"ITPManualPrevalentResource"];
     if (manualPrevalentResource) {
         URL url { URL(), manualPrevalentResource };
@@ -162,7 +164,6 @@ void WebsiteDataStore::platformSetNetworkParameters(WebsiteDataStoreParameters&
 #endif
 
     bool shouldIncludeLocalhostInResourceLoadStatistics = isSafari;
-    bool isInAppBrowserPrivacyEnabled = [defaults boolForKey:[NSString stringWithFormat:@"WebKitDebug%@", WebPreferencesKey::isInAppBrowserPrivacyEnabledKey().createCFString().get()]];
     
     parameters.networkSessionParameters.proxyConfiguration = configuration().proxyConfiguration();
     parameters.networkSessionParameters.sourceApplicationBundleIdentifier = configuration().sourceApplicationBundleIdentifier();
@@ -176,7 +177,6 @@ void WebsiteDataStore::platformSetNetworkParameters(WebsiteDataStoreParameters&
     parameters.networkSessionParameters.alternativeServiceDirectoryExtensionHandle = WTFMove(alternativeServiceStorageDirectoryExtensionHandle);
     parameters.networkSessionParameters.http3Enabled = WTFMove(http3Enabled);
 #endif
-    parameters.networkSessionParameters.isInAppBrowserPrivacyEnabled = isInAppBrowserPrivacyEnabled;
     parameters.networkSessionParameters.resourceLoadStatisticsParameters.shouldIncludeLocalhost = shouldIncludeLocalhostInResourceLoadStatistics;
     parameters.networkSessionParameters.resourceLoadStatisticsParameters.enableDebugMode = enableResourceLoadStatisticsDebugMode;
     parameters.networkSessionParameters.resourceLoadStatisticsParameters.sameSiteStrictEnforcementEnabled = sameSiteStrictEnforcementEnabled;
@@ -405,14 +405,14 @@ void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization forceRein
     
     static const auto maxAppBoundDomainCount = 10;
     
-    appBoundDomainQueue().dispatch([isInAppBrowserPrivacyEnabled = parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled, forceReinitialization] () mutable {
+    appBoundDomainQueue().dispatch([forceReinitialization] () mutable {
         if (hasInitializedAppBoundDomains && forceReinitialization != ForceReinitialization::Yes)
             return;
         
         NSArray<NSString *> *domains = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"WKAppBoundDomains"];
         keyExists = domains ? true : false;
         
-        RunLoop::main().dispatch([isInAppBrowserPrivacyEnabled, forceReinitialization, domains = retainPtr(domains)] {
+        RunLoop::main().dispatch([forceReinitialization, domains = retainPtr(domains)] {
             if (hasInitializedAppBoundDomains && forceReinitialization != ForceReinitialization::Yes)
                 return;
 
@@ -432,9 +432,6 @@ void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization forceRein
                 if (appBoundDomains().size() >= maxAppBoundDomainCount)
                     break;
             }
-            if (isInAppBrowserPrivacyEnabled) {
-                WEBSITE_DATA_STORE_ADDITIONS
-            }
             hasInitializedAppBoundDomains = true;
             if (isAppBoundITPRelaxationEnabled)
                 forwardAppBoundDomainsToITPIfInitialized([] { });
@@ -445,15 +442,21 @@ void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization forceRein
 void WebsiteDataStore::ensureAppBoundDomains(CompletionHandler<void(const HashSet<WebCore::RegistrableDomain>&)>&& completionHandler) const
 {
     if (hasInitializedAppBoundDomains) {
+        if (m_isInAppBrowserPrivacyTestModeEnabled) {
+            WEBSITE_DATA_STORE_ADDITIONS;
+        }
         completionHandler(appBoundDomains());
         return;
     }
 
     // Hopping to the background thread then back to the main thread
     // ensures that initializeAppBoundDomains() has finished.
-    appBoundDomainQueue().dispatch([completionHandler = WTFMove(completionHandler)] () mutable {
-        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] () mutable {
+    appBoundDomainQueue().dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] () mutable {
+        RunLoop::main().dispatch([this, protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)] () mutable {
             ASSERT(hasInitializedAppBoundDomains);
+            if (m_isInAppBrowserPrivacyTestModeEnabled) {
+                WEBSITE_DATA_STORE_ADDITIONS;
+            }
             completionHandler(appBoundDomains());
         });
     });
index 964c631..8b6c55e 100644 (file)
@@ -373,6 +373,7 @@ private:
     WeakHashSet<WebProcessProxy> m_processes;
 
     bool m_isItpStateExplicitlySet { false };
+    bool m_isInAppBrowserPrivacyTestModeEnabled { false };
 
 #if HAVE(SEC_KEY_PROXY)
     Vector<Ref<SecKeyProxyStore>> m_secKeyProxyStores;