Attachment 342832 Details for Bug 186681 – Patch

[patch] Patch

bug-186681-20180615111815.patch (text/plain), 13.16 KB, created by Chris Dumez on 2018-06-15 11:17:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-15 11:17:47 PDT

Size: 13.16 KB

patch

obsolete

>Subversion Revision: 232862
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index aab4aca76353033ed6661399907450198935e9c2..1c9aeb0dcf7db2204b6591475a78279fea56d0c6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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-14  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebCore::SWServer::registrationStoreImportComplete()
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index d79098af114ae760e6c2de2025e26ca25ce5bb12..68b64603acb46cba503a5da89425c592c0901d9a 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-14  Basuke Suzuki  <Basuke.Suzuki@sony.com>
> 
>         [Win] Add IPC error case for broken pipe
>diff --git a/Source/WebCore/workers/service/server/RegistrationDatabase.cpp b/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>index 7749b884cae2ad439f626c919f7c9b9ab3561c9a..e4c3ae8bfbf96443521f790ed901d513a62b0fa1 100644
>--- a/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>+++ b/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>@@ -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));
>diff --git a/Source/WebCore/workers/service/server/RegistrationDatabase.h b/Source/WebCore/workers/service/server/RegistrationDatabase.h
>index 908e2537bf738ac5eedaa993867820d1051ef7f3..e9daf91542bd23b615b22f3b096e997c95fc4184 100644
>--- a/Source/WebCore/workers/service/server/RegistrationDatabase.h
>+++ b/Source/WebCore/workers/service/server/RegistrationDatabase.h
>@@ -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;
>diff --git a/Source/WebCore/workers/service/server/RegistrationStore.cpp b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>index a59b727143f3b5eebdb707f9f6b7e7d356878282..69ae4b99fdbab2f65e1c55a96208fe87e86e2ba0 100644
>--- a/Source/WebCore/workers/service/server/RegistrationStore.cpp
>+++ b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>@@ -41,7 +41,6 @@ RegistrationStore::RegistrationStore(SWServer& server, String&& databaseDirector
> 
> RegistrationStore::~RegistrationStore()
> {
>-    ASSERT(m_database->isClosed());
> }
> 
> void RegistrationStore::scheduleDatabasePushIfNecessary()
>diff --git a/Source/WebKit/StorageProcess/StorageProcess.cpp b/Source/WebKit/StorageProcess/StorageProcess.cpp
>index 19e4be649536f77904511b87bc617cef934b8d20..c4066356bc84606923b550aaf80736f121fbd489 100644
>--- a/Source/WebKit/StorageProcess/StorageProcess.cpp
>+++ b/Source/WebKit/StorageProcess/StorageProcess.cpp
>@@ -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)
>diff --git a/Source/WebKit/StorageProcess/StorageProcess.h b/Source/WebKit/StorageProcess/StorageProcess.h
>index 298390b3d8ac4fd69bdf870487622f1dca4cb194..aee510042be3b5b3c895fe9ae5ad9af73c67548b 100644
>--- a/Source/WebKit/StorageProcess/StorageProcess.h
>+++ b/Source/WebKit/StorageProcess/StorageProcess.h
>@@ -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)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 656706c1f92aecd24a1013e173fe0381230a29f8..f4e2777c787e446ee7ca9277f24bcc32bd07d30f 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+       Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
>+
> 2018-06-14  Basuke Suzuki  <Basuke.Suzuki@sony.com>
> 
>         [Win][MiniBrowser] Change to use WebKit by default if it's available
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm
>index 09d1a4d5f2cadd2922a034058772d4128189c1c3..77cd4ddc8e5e9fe564566fed95d504365b7bae66 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm
>@@ -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

Actions: View | Formatted Diff | Diff

Attachments on bug 186681: 342832