Changeset 238250 in webkit

Timestamp:

Nov 15, 2018 2:01:31 PM (5 years ago)

Author:

Chris Dumez

Message:

IndexedDB.IndexedDBTempFileSize API test times out with process prewarming enabled
​https://bugs.webkit.org/show_bug.cgi?id=191671
<rdar://problem/46086062>

Reviewed by Alex Christensen.

Source/WebKit:

Enabling process prewarming caused IndexedDB.IndexedDBTempFileSize API to time out and print
the following line:
"Attempted to create a NetworkLoad with a session (id=2) that does not exist."

This actually identified a pre-existing bug with our handling of non-default data store.
Whenever a page starts using a data store, we call WebProcessPool::pageBeginUsingWebsiteDataStore()
which will call NetworkProcessProxy::addSession() if the network process was already started
to let the network process know about this non-default session. There are several issues with the
current model:

1. If the network process was not created yet when pageBeginUsingWebsiteDataStore() is called, then the network process will not know about the non-default session when it actually gets started later on. This is unlikely to happen in practice, except in case of network process crash because we create the network process as soon as we initialize the first WebProcessProxy.
2. Even if we successfuly managed to register the session with the network process proxy, we get in trouble if the network process crashes or is terminated later on as we do not re-register those sessions with the new network process.

To address these 2 issues, WebProcessPool::ensureNetworkProcess() now takes care of registering
all the non-default sessions (that are associated with this process pool) with the new network
process. The WebProcessPool knows about these sessions because it adds them to m_sessionToPagesMap
whenever WebProcessPool::pageBeginUsingWebsiteDataStore() is called, even if the network process
proxy was not created yet.

The reason the IndexedDB.IndexedDBTempFileSize API test was failing was because it did:

1. A load in a WebView V1 with a non-default session
2. Process prewarming kicked in after this load and would create a new WebProcessProxy.
3. Terminate the network process
4. Another load in a WebView V2 with the same non-defaut session, which would reuse the prewarmed process. Because the network process was terminated, constructing the new page would not register the session ID with the new network process when pageBeginUsingWebsiteDataStore() is called.

-> The load would hang because the new network process would not know about the

non-default session when started later on.

The bug was previously hidden without process prewarming because step 4 would create a *new*
WebProcessProxy and WebProcessPool::initializeNewWebProcess() would call ensureNetworkProcess()
so that pageBeginUsingWebsiteDataStore() would successfuly register the session with the
network process later on.

I wrote a second API test (WebKit.DoLoadWithNonDefaultDataStoreAfterTerminatingNetworkProcess)
to demonstrate the pre-existing bug without process prewarming enabled.

• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::ensureNetworkProcess):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
• TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:

(TEST):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebProcessPool.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2018-11-15  Chris Dumez  <cdumez@apple.com>
+
+        IndexedDB.IndexedDBTempFileSize API test times out with process prewarming enabled
+        https://bugs.webkit.org/show_bug.cgi?id=191671
+        <rdar://problem/46086062>
+
+        Reviewed by Alex Christensen.
+
+        Enabling process prewarming caused IndexedDB.IndexedDBTempFileSize API to time out and print
+        the following line:
+        "Attempted to create a NetworkLoad with a session (id=2) that does not exist."
+
+        This actually identified a pre-existing bug with our handling of non-default data store.
+        Whenever a page starts using a data store, we call WebProcessPool::pageBeginUsingWebsiteDataStore()
+        which will call NetworkProcessProxy::addSession() if the network process was already started
+        to let the network process know about this non-default session. There are several issues with the
+        current model:
+        1. If the network process was not created yet when pageBeginUsingWebsiteDataStore() is called,
+           then the network process will not know about the non-default session when it actually gets
+           started later on. This is unlikely to happen in practice, except in case of network process
+           crash because we create the network process as soon as we initialize the first WebProcessProxy.
+        2. Even if we successfuly managed to register the session with the network process proxy, we get
+           in trouble if the network process crashes or is terminated later on as we do not re-register
+           those sessions with the new network process.
+
+        To address these 2 issues, WebProcessPool::ensureNetworkProcess() now takes care of registering
+        all the non-default sessions (that are associated with this process pool) with the new network
+        process. The WebProcessPool knows about these sessions because it adds them to m_sessionToPagesMap
+        whenever WebProcessPool::pageBeginUsingWebsiteDataStore() is called, even if the network process
+        proxy was not created yet.
+
+        The reason the IndexedDB.IndexedDBTempFileSize API test was failing was because it did:
+        1. A load in a WebView V1 with a non-default session
+        2. Process prewarming kicked in after this load and would create a new WebProcessProxy.
+        3. Terminate the network process
+        4. Another load in a WebView V2 with the same non-defaut session, which would reuse the
+          prewarmed process. Because the network process was terminated, constructing the new
+          page would not register the session ID with the new network process when
+          pageBeginUsingWebsiteDataStore() is called.
+        -> The load would hang because the new network process would not know about the
+           non-default session when started later on.
+
+        The bug was previously hidden without process prewarming because step 4 would create a *new*
+        WebProcessProxy and WebProcessPool::initializeNewWebProcess() would call ensureNetworkProcess()
+        so that pageBeginUsingWebsiteDataStore() would successfuly register the session with the
+        network process later on.
+
+        I wrote a second API test (WebKit.DoLoadWithNonDefaultDataStoreAfterTerminatingNetworkProcess)
+        to demonstrate the pre-existing bug without process prewarming enabled.
+
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::ensureNetworkProcess):
+
 2018-11-15  Vivek Seth  <v_seth@apple.com>
 

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -569 +569 @@
     }
 
+    // Make sure the network process knowns about all the sessions that have been registered before it started.
+    for (auto& sessionID : m_sessionToPagesMap.keys()) {
+        if (auto* websiteDataStore = WebsiteDataStore::existingNonDefaultDataStoreForSessionID(sessionID))
+            m_networkProcess->addSession(*websiteDataStore);
+    }
+
     if (m_websiteDataStore)
         m_websiteDataStore->websiteDataStore().didCreateNetworkProcess();

trunk/Tools/ChangeLog

@@ +1 @@
+2018-11-15  Chris Dumez  <cdumez@apple.com>
+
+        IndexedDB.IndexedDBTempFileSize API test times out with process prewarming enabled
+        https://bugs.webkit.org/show_bug.cgi?id=191671
+        <rdar://problem/46086062>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+        * TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:
+        (TEST):
+
 2018-11-15  Oriol Brufau  <obrufau@igalia.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -2691 +2691 @@
 }
 
+TEST(ProcessSwap, UsePrewarmedProcessAfterTerminatingNetworkProcess)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    processPoolConfiguration.get().prewarmsProcessesAutomatically = YES;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    webViewConfiguration.get().websiteDataStore = [[[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()] autorelease];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    TestWebKitAPI::Util::spinRunLoop(1);
+
+    [processPool _terminateNetworkProcess];
+
+    auto webView2 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    [webView2 setNavigationDelegate:delegate.get()];
+    request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView2 loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 #if PLATFORM(MAC)
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm

@@ -28 +28 @@
 #import "PlatformUtilities.h"
 #import "Test.h"
-#import <WebKit/WebKit.h>
-
+#import "TestNavigationDelegate.h"
 #import <WebKit/WKProcessPoolPrivate.h>
 #import <WebKit/WKUserContentControllerPrivate.h>
@@ -35 +34 @@
 #import <WebKit/WKWebViewPrivate.h>
 #import <WebKit/WKWebsiteDataStorePrivate.h>
+#import <WebKit/WebKit.h>
 #import <WebKit/_WKProcessPoolConfiguration.h>
 #import <WebKit/_WKUserStyleSheet.h>
@@ -427 +427 @@
 }
 
+TEST(WebKit, DoLoadWithNonDefaultDataStoreAfterTerminatingNetworkProcess)
+{
+    auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    webViewConfiguration.get().websiteDataStore = [[[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()] autorelease];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+    [webView _test_waitForDidFinishNavigation];
+
+    TestWebKitAPI::Util::spinRunLoop(1);
+
+    [webViewConfiguration.get().processPool _terminateNetworkProcess];
+
+    request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+    [webView _test_waitForDidFinishNavigation];
+}
+
 #endif