Service workers do not work well inside Web.app
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Feb 2018 19:02:15 +0000 (19:02 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Feb 2018 19:02:15 +0000 (19:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183105
<rdar://problem/37864140>

Reviewed by Youenn Fablet.

Source/WebCore:

SessionID::defaultSessionID() was hardcoded in the ServiceWorkerThread constructor
instead of using the sessionID of the SWServer that created the service worker thread.
As a result, when the Service Worker would establish a SWClientConnection to the
server, it would use the wrong sessionID and would end up using a different SWServer
(Since we have a different SWServer instance per sessionID). As a result,
ServiceWorkerRegistration / ServiceWorker objects inside the service worker would not
be kept in sync with the server (since they registered themselves with the wrong
SWServer).

Covered by new API test.

* workers/service/ServiceWorkerContextData.cpp:
(WebCore::ServiceWorkerContextData::isolatedCopy const):
* workers/service/ServiceWorkerContextData.h:
(WebCore::ServiceWorkerContextData::encode const):
(WebCore::ServiceWorkerContextData::decode):
* workers/service/context/ServiceWorkerThread.cpp:
(WebCore::ServiceWorkerThread::ServiceWorkerThread):
* workers/service/context/ServiceWorkerThreadProxy.cpp:
(WebCore::createPageForServiceWorker):
(WebCore::ServiceWorkerThreadProxy::ServiceWorkerThreadProxy):
* workers/service/server/RegistrationDatabase.cpp:
(WebCore::RegistrationDatabase::importRecords):
* workers/service/server/RegistrationStore.h:
(WebCore::RegistrationStore::server):
* workers/service/server/SWServer.cpp:
(WebCore::SWServer::updateWorker):
* workers/service/server/SWServerWorker.cpp:
(WebCore::SWServerWorker::contextData const):

Source/WebKit:

* StorageProcess/StorageProcess.cpp:
(WebKit::StorageProcess::didReceiveMessage):
We were failing to forward IPC messages to the ChildProcess class here. As a result,
the ChildProcess::RegisterURLSchemeServiceWorkersCanHandle IPC was being ignored
by the StorageProcess.

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:

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

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/workers/service/ServiceWorkerContextData.cpp
Source/WebCore/workers/service/ServiceWorkerContextData.h
Source/WebCore/workers/service/context/ServiceWorkerThread.cpp
Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp
Source/WebCore/workers/service/server/RegistrationDatabase.cpp
Source/WebCore/workers/service/server/RegistrationStore.h
Source/WebCore/workers/service/server/SWServer.cpp
Source/WebCore/workers/service/server/SWServerWorker.cpp
Source/WebKit/ChangeLog
Source/WebKit/StorageProcess/StorageProcess.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm

index e0be0c5..92b274f 100644 (file)
@@ -1,3 +1,41 @@
+2018-02-25  Chris Dumez  <cdumez@apple.com>
+
+        Service workers do not work well inside Web.app
+        https://bugs.webkit.org/show_bug.cgi?id=183105
+        <rdar://problem/37864140>
+
+        Reviewed by Youenn Fablet.
+
+        SessionID::defaultSessionID() was hardcoded in the ServiceWorkerThread constructor
+        instead of using the sessionID of the SWServer that created the service worker thread.
+        As a result, when the Service Worker would establish a SWClientConnection to the
+        server, it would use the wrong sessionID and would end up using a different SWServer
+        (Since we have a different SWServer instance per sessionID). As a result,
+        ServiceWorkerRegistration / ServiceWorker objects inside the service worker would not
+        be kept in sync with the server (since they registered themselves with the wrong
+        SWServer).
+
+        Covered by new API test.
+
+        * workers/service/ServiceWorkerContextData.cpp:
+        (WebCore::ServiceWorkerContextData::isolatedCopy const):
+        * workers/service/ServiceWorkerContextData.h:
+        (WebCore::ServiceWorkerContextData::encode const):
+        (WebCore::ServiceWorkerContextData::decode):
+        * workers/service/context/ServiceWorkerThread.cpp:
+        (WebCore::ServiceWorkerThread::ServiceWorkerThread):
+        * workers/service/context/ServiceWorkerThreadProxy.cpp:
+        (WebCore::createPageForServiceWorker):
+        (WebCore::ServiceWorkerThreadProxy::ServiceWorkerThreadProxy):
+        * workers/service/server/RegistrationDatabase.cpp:
+        (WebCore::RegistrationDatabase::importRecords):
+        * workers/service/server/RegistrationStore.h:
+        (WebCore::RegistrationStore::server):
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::updateWorker):
+        * workers/service/server/SWServerWorker.cpp:
+        (WebCore::SWServerWorker::contextData const):
+
 2018-02-24  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         Null-dereference of the second argument `resource` of DocumentLoader::scheduleSubstituteResourceLoad
index 4c4bf23..cd09a97 100644 (file)
@@ -32,7 +32,7 @@ namespace WebCore {
 
 ServiceWorkerContextData ServiceWorkerContextData::isolatedCopy() const
 {
-    return { jobDataIdentifier, registration.isolatedCopy(), serviceWorkerIdentifier, script.isolatedCopy(), contentSecurityPolicy.isolatedCopy(), scriptURL.isolatedCopy(), workerType, loadedFromDisk };
+    return { jobDataIdentifier, registration.isolatedCopy(), serviceWorkerIdentifier, script.isolatedCopy(), contentSecurityPolicy.isolatedCopy(), scriptURL.isolatedCopy(), workerType, sessionID, loadedFromDisk };
 }
 
 } // namespace WebCore
index f5a3e09..b93d922 100644 (file)
@@ -31,6 +31,7 @@
 #include "ServiceWorkerRegistrationData.h"
 #include "URL.h"
 #include "WorkerType.h"
+#include <pal/SessionID.h>
 
 #if ENABLE(SERVICE_WORKER)
 
@@ -44,6 +45,7 @@ struct ServiceWorkerContextData {
     ContentSecurityPolicyResponseHeaders contentSecurityPolicy;
     URL scriptURL;
     WorkerType workerType;
+    PAL::SessionID sessionID;
     bool loadedFromDisk;
 
     template<class Encoder> void encode(Encoder&) const;
@@ -55,7 +57,7 @@ struct ServiceWorkerContextData {
 template<class Encoder>
 void ServiceWorkerContextData::encode(Encoder& encoder) const
 {
-    encoder << jobDataIdentifier << registration << serviceWorkerIdentifier << script << contentSecurityPolicy << scriptURL << workerType << loadedFromDisk;
+    encoder << jobDataIdentifier << registration << serviceWorkerIdentifier << script << contentSecurityPolicy << scriptURL << workerType << sessionID << loadedFromDisk;
 }
 
 template<class Decoder>
@@ -91,11 +93,15 @@ std::optional<ServiceWorkerContextData> ServiceWorkerContextData::decode(Decoder
     if (!decoder.decodeEnum(workerType))
         return std::nullopt;
 
+    PAL::SessionID sessionID;
+    if (!decoder.decode(sessionID))
+        return std::nullopt;
+
     bool loadedFromDisk;
     if (!decoder.decode(loadedFromDisk))
         return std::nullopt;
 
-    return {{ WTFMove(*jobDataIdentifier), WTFMove(*registration), WTFMove(*serviceWorkerIdentifier), WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), workerType, loadedFromDisk }};
+    return {{ WTFMove(*jobDataIdentifier), WTFMove(*registration), WTFMove(*serviceWorkerIdentifier), WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), workerType, sessionID, loadedFromDisk }};
 }
 
 } // namespace WebCore
index 6543a1f..f98034e 100644 (file)
@@ -71,7 +71,7 @@ private:
 // FIXME: Use valid runtime flags
 
 ServiceWorkerThread::ServiceWorkerThread(const ServiceWorkerContextData& data, PAL::SessionID, String&& userAgent, WorkerLoaderProxy& loaderProxy, WorkerDebuggerProxy& debuggerProxy, IDBClient::IDBConnectionProxy* idbConnectionProxy, SocketProvider* socketProvider)
-    : WorkerThread(data.scriptURL, "serviceworker:" + Inspector::IdentifiersFactory::createIdentifier(), WTFMove(userAgent), NetworkStateNotifier::singleton().onLine(), data.script, loaderProxy, debuggerProxy, DummyServiceWorkerThreadProxy::shared(), WorkerThreadStartMode::Normal, data.contentSecurityPolicy, false, SecurityOrigin::create(data.scriptURL).get(), MonotonicTime::now(), idbConnectionProxy, socketProvider, JSC::RuntimeFlags::createAllEnabled(), SessionID::defaultSessionID())
+    : WorkerThread(data.scriptURL, "serviceworker:" + Inspector::IdentifiersFactory::createIdentifier(), WTFMove(userAgent), NetworkStateNotifier::singleton().onLine(), data.script, loaderProxy, debuggerProxy, DummyServiceWorkerThreadProxy::shared(), WorkerThreadStartMode::Normal, data.contentSecurityPolicy, false, SecurityOrigin::create(data.scriptURL).get(), MonotonicTime::now(), idbConnectionProxy, socketProvider, JSC::RuntimeFlags::createAllEnabled(), data.sessionID)
     , m_data(data.isolatedCopy())
     , m_workerObjectProxy(DummyServiceWorkerThreadProxy::shared())
 {
index f9e112f..2841930 100644 (file)
@@ -48,9 +48,11 @@ URL static inline topOriginURL(const SecurityOrigin& origin)
     return url;
 }
 
-static inline UniqueRef<Page> createPageForServiceWorker(PageConfiguration&& configuration, const ServiceWorkerContextData& data, SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy)
+static inline UniqueRef<Page> createPageForServiceWorker(PageConfiguration&& configuration, const ServiceWorkerContextData& data, SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy, PAL::SessionID sessionID)
 {
     auto page = makeUniqueRef<Page>(WTFMove(configuration));
+    page->setSessionID(sessionID);
+
     auto& mainFrame = page->mainFrame();
     mainFrame.loader().initForSynthesizedDocument({ });
     auto document = Document::createNonRenderedPlaceholder(&mainFrame, data.scriptURL);
@@ -84,7 +86,7 @@ static HashSet<ServiceWorkerThreadProxy*>& allServiceWorkerThreadProxies()
 }
 
 ServiceWorkerThreadProxy::ServiceWorkerThreadProxy(PageConfiguration&& pageConfiguration, const ServiceWorkerContextData& data, PAL::SessionID sessionID, String&& userAgent, CacheStorageProvider& cacheStorageProvider, SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy)
-    : m_page(createPageForServiceWorker(WTFMove(pageConfiguration), data, storageBlockingPolicy))
+    : m_page(createPageForServiceWorker(WTFMove(pageConfiguration), data, storageBlockingPolicy, data.sessionID))
     , m_document(*m_page->mainFrame().document())
     , m_serviceWorkerThread(ServiceWorkerThread::create(data, sessionID, WTFMove(userAgent), *this, *this, idbConnectionProxy(m_document), m_document->socketProvider()))
     , m_cacheStorageProvider(cacheStorageProvider)
index 6eed9b5..eb1aaf2 100644 (file)
@@ -35,6 +35,7 @@
 #include "SQLiteFileSystem.h"
 #include "SQLiteStatement.h"
 #include "SQLiteTransaction.h"
+#include "SWServer.h"
 #include "SecurityOrigin.h"
 #include <wtf/CompletionHandler.h>
 #include <wtf/MainThread.h>
@@ -355,7 +356,7 @@ String RegistrationDatabase::importRecords()
         auto registrationIdentifier = generateObjectIdentifier<ServiceWorkerRegistrationIdentifierType>();
         auto serviceWorkerData = ServiceWorkerData { workerIdentifier, scriptURL, ServiceWorkerState::Activated, *workerType, registrationIdentifier };
         auto registration = ServiceWorkerRegistrationData { WTFMove(*key), registrationIdentifier, URL(originURL, scopePath), *updateViaCache, lastUpdateCheckTime, std::nullopt, std::nullopt, WTFMove(serviceWorkerData) };
-        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, true };
+        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_store.server().sessionID(), true };
 
         postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::addRegistrationToStore, WTFMove(contextData)));
     }
index 13379d8..29e8214 100644 (file)
@@ -59,6 +59,8 @@ public:
     void databaseFailedToOpen();
     void databaseOpenedAndRecordsImported();
 
+    SWServer& server() { return m_server; };
+
 private:
     void scheduleDatabasePushIfNecessary();
     void pushChangesToDatabase(WTF::CompletionHandler<void()>&&);
index 6c60578..0075f03 100644 (file)
@@ -492,7 +492,7 @@ void SWServer::removeClientServiceWorkerRegistration(Connection& connection, Ser
 
 void SWServer::updateWorker(Connection&, const ServiceWorkerJobDataIdentifier& jobDataIdentifier, SWServerRegistration& registration, const URL& url, const String& script, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicy, WorkerType type)
 {
-    tryInstallContextData({ jobDataIdentifier, registration.data(), generateObjectIdentifier<ServiceWorkerIdentifierType>(), script, contentSecurityPolicy, url, type, false });
+    tryInstallContextData({ jobDataIdentifier, registration.data(), generateObjectIdentifier<ServiceWorkerIdentifierType>(), script, contentSecurityPolicy, url, type, sessionID(), false });
 }
 
 void SWServer::tryInstallContextData(ServiceWorkerContextData&& data)
index e1e6997..968811c 100644 (file)
@@ -73,7 +73,7 @@ ServiceWorkerContextData SWServerWorker::contextData() const
     auto* registration = m_server.getRegistration(m_registrationKey);
     ASSERT(registration);
 
-    return { std::nullopt, registration->data(), m_data.identifier, m_script, m_contentSecurityPolicy, m_data.scriptURL, m_data.type, false };
+    return { std::nullopt, registration->data(), m_data.identifier, m_script, m_contentSecurityPolicy, m_data.scriptURL, m_data.type, m_server.sessionID(), false };
 }
 
 void SWServerWorker::terminate()
index 84007e7..ae2744f 100644 (file)
@@ -1,3 +1,17 @@
+2018-02-25  Chris Dumez  <cdumez@apple.com>
+
+        Service workers do not work well inside Web.app
+        https://bugs.webkit.org/show_bug.cgi?id=183105
+        <rdar://problem/37864140>
+
+        Reviewed by Youenn Fablet.
+
+        * StorageProcess/StorageProcess.cpp:
+        (WebKit::StorageProcess::didReceiveMessage):
+        We were failing to forward IPC messages to the ChildProcess class here. As a result,
+        the ChildProcess::RegisterURLSchemeServiceWorkersCanHandle IPC was being ignored
+        by the StorageProcess.
+
 2018-02-24  Zan Dobersek  <zdobersek@igalia.com>
 
         Unreviewed WPE breakage fix.
index 821a316..e5077b4 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "StorageProcess.h"
 
+#include "ChildProcessMessages.h"
 #include "StorageProcessCreationParameters.h"
 #include "StorageProcessMessages.h"
 #include "StorageProcessProxyMessages.h"
@@ -138,6 +139,11 @@ void StorageProcess::didReceiveMessage(IPC::Connection& connection, IPC::Decoder
         return;
     }
 
+    if (decoder.messageReceiverName() == Messages::ChildProcess::messageReceiverName()) {
+        ChildProcess::didReceiveMessage(connection, decoder);
+        return;
+    }
+
 #if ENABLE(SERVICE_WORKER)
     if (decoder.messageReceiverName() == Messages::WebSWServerToContextConnection::messageReceiverName()) {
         if (auto* swConnection = SWServerToContextConnection::globalServerToContextConnection()) {
index fc70ef4..96f111e 100644 (file)
@@ -1,3 +1,15 @@
+2018-02-25  Chris Dumez  <cdumez@apple.com>
+
+        Service workers do not work well inside Web.app
+        https://bugs.webkit.org/show_bug.cgi?id=183105
+        <rdar://problem/37864140>
+
+        Reviewed by Youenn Fablet.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
+
 2018-02-25  Aakash Jain  <aakash_jain@apple.com>
 
         [build.webkit.org] unit-tests fail if buildbot 0.8.6p1 is not installed locally
index 343f979..60d2503 100644 (file)
@@ -36,6 +36,7 @@
 #import <WebKit/WKWebsiteDataStoreRef.h>
 #import <WebKit/WebKit.h>
 #import <WebKit/_WKExperimentalFeature.h>
+#import <WebKit/_WKWebsiteDataStoreConfiguration.h>
 #import <wtf/Deque.h>
 #import <wtf/HashMap.h>
 #import <wtf/RetainPtr.h>
@@ -342,6 +343,55 @@ navigator.serviceWorker.register('/sw.js').then(function(reg) {
 </script>
 )SWRESOURCE";
 
+static const char* mainBytesForSessionIDTest = R"SWRESOURCE(
+<script>
+
+function log(msg)
+{
+    window.webkit.messageHandlers.sw.postMessage(msg);
+}
+
+navigator.serviceWorker.addEventListener("message", function(event) {
+    log(event.data);
+});
+
+try {
+
+navigator.serviceWorker.register('/sw.js').then(function(reg) {
+    worker = reg.installing;
+    worker.addEventListener('statechange', function() {
+        if (worker.state == 'activated')
+            worker.postMessage("TEST");
+    });
+}).catch(function(error) {
+    log("Registration failed with: " + error);
+});
+} catch(e) {
+    log("Exception: " + e);
+}
+
+</script>
+)SWRESOURCE";
+
+static const char* scriptBytesForSessionIDTest = R"SWRESOURCE(
+
+var wasActivated = false;
+
+self.addEventListener("activate", event => {
+    event.waitUntil(clients.claim().then( () => {
+        wasActivated = true;
+    }));
+});
+
+self.addEventListener("message", (event) => {
+    if (wasActivated && registration.active)
+        event.source.postMessage("PASS: activation successful");
+    else
+        event.source.postMessage("FAIL: failed to activate");
+});
+
+)SWRESOURCE";
+
 TEST(ServiceWorkers, Basic)
 {
     ASSERT(mainBytes);
@@ -928,7 +978,57 @@ TEST(ServiceWorkers, HasServiceWorkerRegistrationBit)
     TestWebKitAPI::Util::run(&done);
     done = false;
 }
-
 #endif // WK_HAVE_C_SPI
 
+TEST(ServiceWorkers, NonDefaultSessionID)
+{
+    [WKWebsiteDataStore _allowWebsiteDataRecordsForAllOrigins];
+
+    NSURL *serviceWorkersPath = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/CustomWebsiteData/ServiceWorkers/" stringByExpandingTildeInPath] isDirectory:YES];
+    [[NSFileManager defaultManager] removeItemAtURL:serviceWorkersPath error:nil];
+    EXPECT_FALSE([[NSFileManager defaultManager] fileExistsAtPath:serviceWorkersPath.path]);
+    NSURL *idbPath = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/CustomWebsiteData/IndexedDB/" stringByExpandingTildeInPath] isDirectory:YES];
+    [[NSFileManager defaultManager] removeItemAtURL:idbPath error:nil];
+    EXPECT_FALSE([[NSFileManager defaultManager] fileExistsAtPath:idbPath.path]);
+
+    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    RetainPtr<_WKWebsiteDataStoreConfiguration> websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+    websiteDataStoreConfiguration.get()._serviceWorkerRegistrationDirectory = serviceWorkersPath;
+    websiteDataStoreConfiguration.get()._indexedDBDatabaseDirectory = idbPath;
+
+    configuration.get().websiteDataStore = [[[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()] autorelease];
+
+    RetainPtr<SWMessageHandlerWithExpectedMessage> messageHandler = adoptNS([[SWMessageHandlerWithExpectedMessage alloc] init]);
+    [[configuration userContentController] addScriptMessageHandler:messageHandler.get() name:@"sw"];
+
+    RetainPtr<SWSchemes> handler = adoptNS([[SWSchemes alloc] init]);
+    handler->resources.set("sw://host/main.html", ResourceInfo { @"text/html", mainBytesForSessionIDTest });
+    handler->resources.set("sw://host/sw.js", ResourceInfo { @"application/javascript", scriptBytesForSessionIDTest });
+    [configuration setURLSchemeHandler:handler.get() forURLScheme:@"SW"];
+
+    expectedMessage = "PASS: activation successful";
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+    [webView.get().configuration.processPool _registerURLSchemeServiceWorkersCanHandle:@"sw"];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"sw://host/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    webView = nullptr;
+
+    EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:serviceWorkersPath.path]);
+
+    [configuration.get().websiteDataStore fetchDataRecordsOfTypes:[NSSet setWithObject:WKWebsiteDataTypeServiceWorkerRegistrations] completionHandler:^(NSArray<WKWebsiteDataRecord *> *websiteDataRecords) {
+        EXPECT_EQ(1u, [websiteDataRecords count]);
+        EXPECT_TRUE([websiteDataRecords[0].displayName isEqualToString:@"sw host"]);
+
+        done = true;
+    }];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 #endif // WK_API_ENABLED