Replace WebsiteDataStoreParameters::privateSessionParameters with re-initializing...
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Sep 2019 19:16:36 +0000 (19:16 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Sep 2019 19:16:36 +0000 (19:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202211

Reviewed by Tim Horton.

Source/WebKit:

Re-adding an ephemeral session after a NetworkProcess crash based on guessing that all its parameters are empty was added in r227590 with a test.
That test passes even when that re-adding code is removed because we re-add all sessions with parameters from the UIProcess when we restart a NetworkProcess.
I move the addition of non-default sessions to the initialization message instead of messages after the initialization message to remove race conditions that
might cause loads to happen before the NetworkProcess has received its second message from the UIProcess.  I also add a unit test to verify that session
resumption also works with non-default persistent WebsiteDataStores.

* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::initializeNetworkProcess):
* NetworkProcess/NetworkProcessCreationParameters.cpp:
(WebKit::NetworkProcessCreationParameters::encode const):
(WebKit::NetworkProcessCreationParameters::decode):
* NetworkProcess/NetworkProcessCreationParameters.h:
* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::startNetworkLoad):
* NetworkProcess/NetworkSessionCreationParameters.cpp:
(WebKit::NetworkSessionCreationParameters::privateSessionParameters): Deleted.
* NetworkProcess/NetworkSessionCreationParameters.h:
* Shared/WebsiteDataStoreParameters.cpp:
(WebKit::WebsiteDataStoreParameters::privateSessionParameters): Deleted.
* Shared/WebsiteDataStoreParameters.h:
(WebKit::WebsiteDataStoreParameters::legacyPrivateSessionParameters): Deleted.
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::ensureNetworkProcess):

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/NetworkProcessCrashNonPersistentDataStore.mm:
(checkRecoveryAfterCrash):
(TEST):

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

12 files changed:
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkProcess.cpp
Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.cpp
Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.h
Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp
Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h
Source/WebKit/Shared/WebsiteDataStoreParameters.cpp
Source/WebKit/Shared/WebsiteDataStoreParameters.h
Source/WebKit/UIProcess/WebProcessPool.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcessCrashNonPersistentDataStore.mm

index 1e67cbd..4a1e76e 100644 (file)
@@ -1,5 +1,36 @@
 2019-09-25  Alex Christensen  <achristensen@webkit.org>
 
+        Replace WebsiteDataStoreParameters::privateSessionParameters with re-initializing all sessions immediately upon NetworkProcess resumption
+        https://bugs.webkit.org/show_bug.cgi?id=202211
+
+        Reviewed by Tim Horton.
+
+        Re-adding an ephemeral session after a NetworkProcess crash based on guessing that all its parameters are empty was added in r227590 with a test.
+        That test passes even when that re-adding code is removed because we re-add all sessions with parameters from the UIProcess when we restart a NetworkProcess.
+        I move the addition of non-default sessions to the initialization message instead of messages after the initialization message to remove race conditions that
+        might cause loads to happen before the NetworkProcess has received its second message from the UIProcess.  I also add a unit test to verify that session
+        resumption also works with non-default persistent WebsiteDataStores.
+
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::initializeNetworkProcess):
+        * NetworkProcess/NetworkProcessCreationParameters.cpp:
+        (WebKit::NetworkProcessCreationParameters::encode const):
+        (WebKit::NetworkProcessCreationParameters::decode):
+        * NetworkProcess/NetworkProcessCreationParameters.h:
+        * NetworkProcess/NetworkResourceLoader.cpp:
+        (WebKit::NetworkResourceLoader::startNetworkLoad):
+        * NetworkProcess/NetworkSessionCreationParameters.cpp:
+        (WebKit::NetworkSessionCreationParameters::privateSessionParameters): Deleted.
+        * NetworkProcess/NetworkSessionCreationParameters.h:
+        * Shared/WebsiteDataStoreParameters.cpp:
+        (WebKit::WebsiteDataStoreParameters::privateSessionParameters): Deleted.
+        * Shared/WebsiteDataStoreParameters.h:
+        (WebKit::WebsiteDataStoreParameters::legacyPrivateSessionParameters): Deleted.
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::ensureNetworkProcess):
+
+2019-09-25  Alex Christensen  <achristensen@webkit.org>
+
         Replace _WKProcessPoolConfiguration.CTDataConnectionServiceType with _WKWebsiteDataStoreConfiguration.dataConnectionServiceType
         https://bugs.webkit.org/show_bug.cgi?id=202174
 
index 4d3e19c..09c80bc 100644 (file)
@@ -322,6 +322,8 @@ void NetworkProcess::initializeNetworkProcess(NetworkProcessCreationParameters&&
 
     auto sessionID = parameters.defaultDataStoreParameters.networkSessionParameters.sessionID;
     setSession(sessionID, NetworkSession::create(*this, WTFMove(parameters.defaultDataStoreParameters.networkSessionParameters)));
+    for (auto&& parameters : WTFMove(parameters.nonDefaultDataStoreParameters))
+        addWebsiteDataStore(WTFMove(parameters));
 
 #if ENABLE(INDEXED_DATABASE)
     addIndexedDatabaseSession(sessionID, parameters.defaultDataStoreParameters.indexedDatabaseDirectory, parameters.defaultDataStoreParameters.indexedDatabaseDirectoryExtensionHandle);
index c0902df..845cea3 100644 (file)
@@ -66,6 +66,7 @@ void NetworkProcessCreationParameters::encode(IPC::Encoder& encoder) const
     encoder << suppressesConnectionTerminationOnSystemChange;
 #endif
     encoder << defaultDataStoreParameters;
+    encoder << nonDefaultDataStoreParameters;
 #if USE(SOUP)
     encoder.encodeEnum(cookieAcceptPolicy);
     encoder << ignoreTLSErrors;
@@ -155,6 +156,12 @@ bool NetworkProcessCreationParameters::decode(IPC::Decoder& decoder, NetworkProc
         return false;
     result.defaultDataStoreParameters = WTFMove(*defaultDataStoreParameters);
 
+    Optional<Vector<WebsiteDataStoreParameters>> nonDefaultDataStoreParameters;
+    decoder >> nonDefaultDataStoreParameters;
+    if (!nonDefaultDataStoreParameters)
+        return false;
+    result.nonDefaultDataStoreParameters = WTFMove(*nonDefaultDataStoreParameters);
+    
 #if USE(SOUP)
     if (!decoder.decodeEnum(result.cookieAcceptPolicy))
         return false;
index 19a9623..242dd6b 100644 (file)
@@ -82,6 +82,7 @@ struct NetworkProcessCreationParameters {
 #endif
 
     WebsiteDataStoreParameters defaultDataStoreParameters;
+    Vector<WebsiteDataStoreParameters> nonDefaultDataStoreParameters;
     
 #if USE(SOUP)
     HTTPCookieAcceptPolicy cookieAcceptPolicy { HTTPCookieAcceptPolicy::AlwaysAccept };
index e4015c9..11cc76c 100644 (file)
@@ -302,10 +302,6 @@ void NetworkResourceLoader::startNetworkLoad(ResourceRequest&& request, FirstLoa
         parameters.storedCredentialsPolicy = m_networkLoadChecker->storedCredentialsPolicy();
 
     auto* networkSession = m_connection->networkSession();
-    if (!networkSession && sessionID().isEphemeral()) {
-        m_connection->networkProcess().addWebsiteDataStore(WebsiteDataStoreParameters::privateSessionParameters(sessionID()));
-        networkSession = m_connection->networkProcess().networkSession(sessionID());
-    }
     if (!networkSession) {
         WTFLogAlways("Attempted to create a NetworkLoad with a session (id=%" PRIu64 ") that does not exist.", sessionID().toUInt64());
         RELEASE_LOG_ERROR_IF_ALLOWED("startNetworkLoad: Attempted to create a NetworkLoad with a session that does not exist (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ", sessionID=%" PRIu64 ")", m_parameters.webPageID.toUInt64(), m_parameters.webFrameID.toUInt64(), m_parameters.identifier, sessionID().toUInt64());
index 319daf7..e90f066 100644 (file)
 
 namespace WebKit {
 
-NetworkSessionCreationParameters NetworkSessionCreationParameters::privateSessionParameters(const PAL::SessionID& sessionID)
-{
-    return {
-        sessionID
-        , { }
-        , AllowsCellularAccess::Yes
-#if PLATFORM(COCOA)
-        , { }
-        , { }
-        , { }
-        , false
-        , { }
-        , { }
-        , { }
-        , false
-#endif
-#if USE(SOUP)
-        , { }
-        , SoupCookiePersistentStorageType::Text
-#endif
-#if USE(CURL)
-        , { }
-        , { }
-#endif
-        , { }
-        , { }
-        , false
-        , false
-        , { }
-        , { }
-        , { }
-        , { }
-        , { }
-        , { }
-        , { }
-        , { }
-        , { }
-    };
-}
-
 void NetworkSessionCreationParameters::encode(IPC::Encoder& encoder) const
 {
     encoder << sessionID;
index 8598c4b..31cde55 100644 (file)
@@ -57,7 +57,6 @@ enum class AllowsCellularAccess : bool { No, Yes };
 struct NetworkSessionCreationParameters {
     void encode(IPC::Encoder&) const;
     static Optional<NetworkSessionCreationParameters> decode(IPC::Decoder&);
-    static NetworkSessionCreationParameters privateSessionParameters(const PAL::SessionID&);
     
     PAL::SessionID sessionID { PAL::SessionID::defaultSessionID() };
     String boundInterfaceIdentifier;
index f18dd16..2709db5 100644 (file)
@@ -150,21 +150,4 @@ Optional<WebsiteDataStoreParameters> WebsiteDataStoreParameters::decode(IPC::Dec
     return parameters;
 }
 
-WebsiteDataStoreParameters WebsiteDataStoreParameters::privateSessionParameters(PAL::SessionID sessionID)
-{
-    ASSERT(sessionID.isEphemeral());
-    return { { }, { }, { }, NetworkSessionCreationParameters::privateSessionParameters(sessionID)
-#if ENABLE(INDEXED_DATABASE)
-        , { }, { }
-#if PLATFORM(IOS_FAMILY)
-        , { }
-#endif
-#endif
-#if ENABLE(SERVICE_WORKER)
-        , { }, { }
-#endif
-        , { }, { }
-    };
-}
-
 } // namespace WebKit
index 6fece3f..062e8f5 100644 (file)
@@ -46,9 +46,6 @@ struct WebsiteDataStoreParameters {
     WebsiteDataStoreParameters& operator=(WebsiteDataStoreParameters&&) = default;
     ~WebsiteDataStoreParameters();
 
-    static WebsiteDataStoreParameters legacyPrivateSessionParameters() { return privateSessionParameters(PAL::SessionID::legacyPrivateSessionID()); }
-    static WebsiteDataStoreParameters privateSessionParameters(PAL::SessionID);
-
     void encode(IPC::Encoder&) const;
     static Optional<WebsiteDataStoreParameters> decode(IPC::Decoder&);
 
index 3d54228..895330d 100644 (file)
@@ -633,6 +633,12 @@ NetworkProcessProxy& WebProcessPool::ensureNetworkProcess(WebsiteDataStore* with
     // Add any platform specific parameters
     platformInitializeNetworkProcess(parameters);
 
+    // Make sure the network process knows about all the sessions that have been registered before it started.
+    for (auto& sessionID : m_sessionToPageIDsMap.keys()) {
+        if (auto* websiteDataStore = WebsiteDataStore::existingNonDefaultDataStoreForSessionID(sessionID))
+            parameters.nonDefaultDataStoreParameters.append(websiteDataStore->parameters());
+    }
+
     // Initialize the network process.
     networkProcess->send(Messages::NetworkProcess::InitializeNetworkProcess(parameters), 0);
 
@@ -650,12 +656,6 @@ NetworkProcessProxy& WebProcessPool::ensureNetworkProcess(WebsiteDataStore* with
         withWebsiteDataStore->clearPendingCookies();
     }
 
-    // Make sure the network process knows about all the sessions that have been registered before it started.
-    for (auto& sessionID : m_sessionToPageIDsMap.keys()) {
-        if (auto* websiteDataStore = WebsiteDataStore::existingNonDefaultDataStoreForSessionID(sessionID))
-            networkProcess->addSession(*websiteDataStore);
-    }
-
     m_networkProcess = WTFMove(networkProcess);
     return *m_networkProcess;
 }
index 0a8a9f3..b9b2784 100644 (file)
@@ -1,3 +1,14 @@
+2019-09-25  Alex Christensen  <achristensen@webkit.org>
+
+        Replace WebsiteDataStoreParameters::privateSessionParameters with re-initializing all sessions immediately upon NetworkProcess resumption
+        https://bugs.webkit.org/show_bug.cgi?id=202211
+
+        Reviewed by Tim Horton.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/NetworkProcessCrashNonPersistentDataStore.mm:
+        (checkRecoveryAfterCrash):
+        (TEST):
+
 2019-09-25  Aakash Jain  <aakash_jain@apple.com>
 
         [EWS] JSC queues should dynamically add required build steps for re-testing the patch
index f4f9055..ee67d2b 100644 (file)
@@ -29,6 +29,8 @@
 #import "Test.h"
 #import "TestWKWebView.h"
 #import <WebKit/WKProcessPoolPrivate.h>
+#import <WebKit/WKWebsiteDataStorePrivate.h>
+#import <WebKit/_WKWebsiteDataStoreConfiguration.h>
 #import <wtf/RetainPtr.h>
 
 static bool done;
@@ -55,13 +57,13 @@ static bool done;
 
 @end
 
-TEST(WebKit, NetworkProcessCrashNonPersistentDataStore)
+static void checkRecoveryAfterCrash(WKWebsiteDataStore *dataStore)
 {
     NSURL *simple = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     NSURL *simple2 = [[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     
     auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
-    [configuration setWebsiteDataStore:[WKWebsiteDataStore nonPersistentDataStore]];
+    [configuration setWebsiteDataStore:dataStore];
     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
     auto delegate = adoptNS([[CrashDelegate alloc] init]);
     [webView setNavigationDelegate:delegate.get()];
@@ -74,3 +76,13 @@ TEST(WebKit, NetworkProcessCrashNonPersistentDataStore)
     [webView loadRequest:[NSURLRequest requestWithURL:simple2]];
     TestWebKitAPI::Util::run(&done);
 }
+
+TEST(WebKit, NetworkProcessCrashNonPersistentDataStore)
+{
+    checkRecoveryAfterCrash([WKWebsiteDataStore nonPersistentDataStore]);
+}
+
+TEST(WebKit, NetworkProcessCrashNonDefaultPersistentDataStore)
+{
+    checkRecoveryAfterCrash([[[WKWebsiteDataStore alloc] _initWithConfiguration:[[[_WKWebsiteDataStoreConfiguration alloc] init] autorelease]] autorelease]);
+}