Add API test coverage for SW RegistrationDatabase destruction and fix issues found...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 20:11:41 +0000 (20:11 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 20:11:41 +0000 (20:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186681

Reviewed by Brady Eidson.

Source/WebCore:

* workers/service/server/RegistrationDatabase.cpp:
(WebCore::RegistrationDatabase::RegistrationDatabase):
(WebCore::RegistrationDatabase::importRecords):
* workers/service/server/RegistrationDatabase.h:
Rename m_session to m_sessionID for clarity.

* workers/service/server/RegistrationStore.cpp:
(WebCore::RegistrationStore::~RegistrationStore):
Drop bad assertion now that the RegistrationDatabase is refcounted
and can outlive the RegistrationStore. The RegistrationDatabase will
take care of closing / destroying the SQLiteDatabase on the background
thread when destroyed.

Source/WebKit:

Make sure StorageProcess::unregisterSWServerConnection() does not unnecessarily
create a SWServer. Otherwise, we were in quick session destroying the SWServer
and then re-constructing it for the same sessionID, merely to try ot unregister
a SWServerConnection.

* StorageProcess/StorageProcess.cpp:
(WebKit::StorageProcess::existingSWOriginStoreForSession const):
(WebKit::StorageProcess::unregisterSWServerConnection):
* StorageProcess/StorageProcess.h:

Tools:

       Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:

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

Source/WebCore/ChangeLog
Source/WebCore/workers/service/server/RegistrationDatabase.cpp
Source/WebCore/workers/service/server/RegistrationDatabase.h
Source/WebCore/workers/service/server/RegistrationStore.cpp
Source/WebKit/ChangeLog
Source/WebKit/StorageProcess/StorageProcess.cpp
Source/WebKit/StorageProcess/StorageProcess.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm

index 0e9edfb..29a1f76 100644 (file)
@@ -1,3 +1,23 @@
+2018-06-15  Chris Dumez  <cdumez@apple.com>
+
+        Add API test coverage for SW RegistrationDatabase destruction and fix issues found by the test
+        https://bugs.webkit.org/show_bug.cgi?id=186681
+
+        Reviewed by Brady Eidson.
+
+        * workers/service/server/RegistrationDatabase.cpp:
+        (WebCore::RegistrationDatabase::RegistrationDatabase):
+        (WebCore::RegistrationDatabase::importRecords):
+        * workers/service/server/RegistrationDatabase.h:
+        Rename m_session to m_sessionID for clarity.
+
+        * workers/service/server/RegistrationStore.cpp:
+        (WebCore::RegistrationStore::~RegistrationStore):
+        Drop bad assertion now that the RegistrationDatabase is refcounted
+        and can outlive the RegistrationStore. The RegistrationDatabase will
+        take care of closing / destroying the SQLiteDatabase on the background
+        thread when destroyed.
+
 2018-06-15  Timothy Hatcher  <timothy@apple.com>
 
         REGRESSION (r232799): Form controls are blank in dark mode.
index 7749b88..e4c3ae8 100644 (file)
@@ -95,7 +95,7 @@ static inline void cleanOldDatabases(const String& databaseDirectory)
 RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, String&& databaseDirectory)
     : m_workQueue(WorkQueue::create("ServiceWorker I/O Thread", WorkQueue::Type::Serial))
     , m_store(makeWeakPtr(store))
-    , m_session(m_store->server().sessionID())
+    , m_sessionID(m_store->server().sessionID())
     , m_databaseDirectory(WTFMove(databaseDirectory))
     , m_databaseFilePath(FileSystem::pathByAppendingComponent(m_databaseDirectory, databaseFilename()))
 {
@@ -404,7 +404,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, m_session, true, WTFMove(scriptResourceMap) };
+        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_sessionID, true, WTFMove(scriptResourceMap) };
 
         callOnMainThread([protectedThis = makeRef(*this), contextData = contextData.isolatedCopy()]() mutable {
             protectedThis->addRegistrationToStore(WTFMove(contextData));
index 908e253..e9daf91 100644 (file)
@@ -77,7 +77,7 @@ private:
 
     Ref<WorkQueue> m_workQueue;
     WeakPtr<RegistrationStore> m_store;
-    PAL::SessionID m_session;
+    PAL::SessionID m_sessionID;
     String m_databaseDirectory;
     String m_databaseFilePath;
     std::unique_ptr<SQLiteDatabase> m_database;
index a59b727..69ae4b9 100644 (file)
@@ -41,7 +41,6 @@ RegistrationStore::RegistrationStore(SWServer& server, String&& databaseDirector
 
 RegistrationStore::~RegistrationStore()
 {
-    ASSERT(m_database->isClosed());
 }
 
 void RegistrationStore::scheduleDatabasePushIfNecessary()
index 406d457..fc46f51 100644 (file)
@@ -1,3 +1,20 @@
+2018-06-15  Chris Dumez  <cdumez@apple.com>
+
+        Add API test coverage for SW RegistrationDatabase destruction and fix issues found by the test
+        https://bugs.webkit.org/show_bug.cgi?id=186681
+
+        Reviewed by Brady Eidson.
+
+        Make sure StorageProcess::unregisterSWServerConnection() does not unnecessarily
+        create a SWServer. Otherwise, we were in quick session destroying the SWServer
+        and then re-constructing it for the same sessionID, merely to try ot unregister
+        a SWServerConnection.
+
+        * StorageProcess/StorageProcess.cpp:
+        (WebKit::StorageProcess::existingSWOriginStoreForSession const):
+        (WebKit::StorageProcess::unregisterSWServerConnection):
+        * StorageProcess/StorageProcess.h:
+
 2018-06-15  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 
         [WinCairo] Move unrelated features of WorkQueueWin into IPC::Connection
index 19e4be6..c406635 100644 (file)
@@ -456,6 +456,14 @@ WebSWOriginStore& StorageProcess::swOriginStoreForSession(PAL::SessionID session
     return static_cast<WebSWOriginStore&>(swServerForSession(sessionID).originStore());
 }
 
+WebSWOriginStore* StorageProcess::existingSWOriginStoreForSession(PAL::SessionID sessionID) const
+{
+    auto* swServer = m_swServers.get(sessionID);
+    if (!swServer)
+        return nullptr;
+    return &static_cast<WebSWOriginStore&>(swServer->originStore());
+}
+
 WebSWServerToContextConnection* StorageProcess::serverToContextConnectionForOrigin(const SecurityOriginData& securityOrigin)
 {
     return m_serverToContextConnections.get(securityOrigin);
@@ -533,7 +541,8 @@ void StorageProcess::unregisterSWServerConnection(WebSWServerConnection& connect
 {
     ASSERT(m_swServerConnections.get(connection.identifier()) == &connection);
     m_swServerConnections.remove(connection.identifier());
-    swOriginStoreForSession(connection.sessionID()).unregisterSWServerConnection(connection);
+    if (auto* store = existingSWOriginStoreForSession(connection.sessionID()))
+        store->unregisterSWServerConnection(connection);
 }
 
 void StorageProcess::swContextConnectionMayNoLongerBeNeeded(WebSWServerToContextConnection& serverToContextConnection)
index 298390b..aee5100 100644 (file)
@@ -151,6 +151,7 @@ private:
     void disableServiceWorkerProcessTerminationDelay();
 
     WebSWOriginStore& swOriginStoreForSession(PAL::SessionID);
+    WebSWOriginStore* existingSWOriginStoreForSession(PAL::SessionID) const;
     bool needsServerToContextConnectionForOrigin(const WebCore::SecurityOriginData&) const;
 #endif
 #if ENABLE(INDEXED_DATABASE)
index ea058ab..b131b8a 100644 (file)
@@ -1,3 +1,14 @@
+2018-06-15  Chris Dumez  <cdumez@apple.com>
+
+        Add API test coverage for SW RegistrationDatabase destruction and fix issues found by the test
+        https://bugs.webkit.org/show_bug.cgi?id=186681
+
+        Reviewed by Brady Eidson.
+
+       Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
+
 2018-06-15  Brady Eidson  <beidson@apple.com>
 
         Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
index 09d1a4d..77cd4dd 100644 (file)
@@ -1418,4 +1418,75 @@ TEST(ServiceWorkers, LoadData)
     done = false;
 }
 
+TEST(ServiceWorkers, RestoreFromDiskNonDefaultStore)
+{
+    ASSERT(mainRegisteringWorkerBytes);
+    ASSERT(scriptBytes);
+    ASSERT(mainRegisteringAlreadyExistingWorkerBytes);
+
+    [WKWebsiteDataStore _allowWebsiteDataRecordsForAllOrigins];
+
+    NSURL *swDBPath = [NSURL fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/CustomWebsiteData/ServiceWorkers/" stringByExpandingTildeInPath]];
+    [[NSFileManager defaultManager] removeItemAtURL:swDBPath error:nil];
+    EXPECT_FALSE([[NSFileManager defaultManager] fileExistsAtPath:swDBPath.path]);
+    [[NSFileManager defaultManager] createDirectoryAtURL:swDBPath withIntermediateDirectories:YES attributes:nil error:nil];
+    EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:swDBPath.path]);
+
+    // We protect the process pool so that it outlives the WebsiteDataStore.
+    RetainPtr<WKProcessPool> protectedProcessPool;
+
+    @autoreleasepool {
+        auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+
+        auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+        websiteDataStoreConfiguration.get()._serviceWorkerRegistrationDirectory = swDBPath;
+        auto nonDefaultDataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
+        configuration.get().websiteDataStore = nonDefaultDataStore.get();
+
+        auto messageHandler = adoptNS([[SWMessageHandlerForRestoreFromDiskTest alloc] initWithExpectedMessage:@"PASS: Registration was successful and service worker was activated"]);
+        [[configuration userContentController] addScriptMessageHandler:messageHandler.get() name:@"sw"];
+
+        auto handler = adoptNS([[SWSchemes alloc] init]);
+        handler->resources.set("sw://host/main.html", ResourceInfo { @"text/html", mainRegisteringWorkerBytes });
+        handler->resources.set("sw://host/sw.js", ResourceInfo { @"application/javascript", scriptBytes });
+        [configuration setURLSchemeHandler:handler.get() forURLScheme:@"SW"];
+
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+        [webView.get().configuration.processPool _registerURLSchemeServiceWorkersCanHandle:@"sw"];
+        protectedProcessPool = webView.get().configuration.processPool;
+
+        [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"sw://host/main.html"]]];
+
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+
+        [webView.get().configuration.processPool _terminateServiceWorkerProcesses];
+    }
+
+    @autoreleasepool {
+        auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+
+        auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+        websiteDataStoreConfiguration.get()._serviceWorkerRegistrationDirectory = swDBPath;
+        auto nonDefaultDataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
+        configuration.get().websiteDataStore = nonDefaultDataStore.get();
+
+        auto messageHandler = adoptNS([[SWMessageHandlerForRestoreFromDiskTest alloc] initWithExpectedMessage:@"PASS: Registration already has an active worker"]);
+        [[configuration userContentController] addScriptMessageHandler:messageHandler.get() name:@"sw"];
+
+        auto handler = adoptNS([[SWSchemes alloc] init]);
+        handler->resources.set("sw://host/main.html", ResourceInfo { @"text/html", mainRegisteringAlreadyExistingWorkerBytes });
+        handler->resources.set("sw://host/sw.js", ResourceInfo { @"application/javascript", scriptBytes });
+        [configuration setURLSchemeHandler:handler.get() forURLScheme:@"SW"];
+
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+        [webView.get().configuration.processPool _registerURLSchemeServiceWorkersCanHandle:@"sw"];
+
+        [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"sw://host/main.html"]]];
+
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+    }
+}
+
 #endif // WK_API_ENABLED