Title: [229183] trunk/Source
Revision
229183
Author
commit-qu...@webkit.org
Date
2018-03-02 11:17:02 -0800 (Fri, 02 Mar 2018)

Log Message

Clients should register to StorageProcess with their service worker registration identifier
https://bugs.webkit.org/show_bug.cgi?id=182313
<rdar://problem/38044403>

Patch by Youenn Fablet <you...@apple.com> on 2018-03-02
Reviewed by Chris Dumez.

Source/WebCore:

Relanding with fixing matchAll for uncontrolled clients.

No observable change of behavior in regular conditions.
When service worker process crashes, the service worker identifiers sent by the WebProcess might be wrong
and we will not be able to retrieve the registration from these identifiers.
The storage process will be able to still process correctly messages coming from the WebProcess to register clients of the registration.
Otherwise, there is a chance that WebProcess clients will not be added to the SWServerRegistration.m_clientsUsingRegistration maps.

* dom/Document.cpp:
(WebCore::Document::setServiceWorkerConnection):
* workers/service/SWClientConnection.h:
* workers/service/server/SWServer.cpp:
(WebCore::SWServer::matchAll):
(WebCore::SWServer::claim):
(WebCore::SWServer::registerServiceWorkerClient):
(WebCore::SWServer::unregisterServiceWorkerClient):
(WebCore::SWServer::setClientActiveWorker): Deleted.
* workers/service/server/SWServer.h:
* workers/service/server/SWServerRegistration.cpp:
(WebCore::SWServerRegistration::activate):

Source/WebKit:

Relanding.

* StorageProcess/ServiceWorker/WebSWServerConnection.cpp:
(WebKit::WebSWServerConnection::registerServiceWorkerClient):
* StorageProcess/ServiceWorker/WebSWServerConnection.h:
* StorageProcess/ServiceWorker/WebSWServerConnection.messages.in:
* WebProcess/Storage/WebSWClientConnection.cpp:
(WebKit::WebSWClientConnection::registerServiceWorkerClient):
* WebProcess/Storage/WebSWClientConnection.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (229182 => 229183)


--- trunk/Source/WebCore/ChangeLog	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/ChangeLog	2018-03-02 19:17:02 UTC (rev 229183)
@@ -1,5 +1,34 @@
 2018-03-02  Youenn Fablet  <you...@apple.com>
 
+        Clients should register to StorageProcess with their service worker registration identifier
+        https://bugs.webkit.org/show_bug.cgi?id=182313
+        <rdar://problem/38044403>
+
+        Reviewed by Chris Dumez.
+
+        Relanding with fixing matchAll for uncontrolled clients.
+
+        No observable change of behavior in regular conditions.
+        When service worker process crashes, the service worker identifiers sent by the WebProcess might be wrong
+        and we will not be able to retrieve the registration from these identifiers.
+        The storage process will be able to still process correctly messages coming from the WebProcess to register clients of the registration.
+        Otherwise, there is a chance that WebProcess clients will not be added to the SWServerRegistration.m_clientsUsingRegistration maps.
+
+        * dom/Document.cpp:
+        (WebCore::Document::setServiceWorkerConnection):
+        * workers/service/SWClientConnection.h:
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::matchAll):
+        (WebCore::SWServer::claim):
+        (WebCore::SWServer::registerServiceWorkerClient):
+        (WebCore::SWServer::unregisterServiceWorkerClient):
+        (WebCore::SWServer::setClientActiveWorker): Deleted.
+        * workers/service/server/SWServer.h:
+        * workers/service/server/SWServerRegistration.cpp:
+        (WebCore::SWServerRegistration::activate):
+
+2018-03-02  Youenn Fablet  <you...@apple.com>
+
         Loads for a Document controlled by a Service Worker should not use AppCache
         https://bugs.webkit.org/show_bug.cgi?id=183148
 

Modified: trunk/Source/WebCore/dom/Document.cpp (229182 => 229183)


--- trunk/Source/WebCore/dom/Document.cpp	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/dom/Document.cpp	2018-03-02 19:17:02 UTC (rev 229183)
@@ -7743,8 +7743,8 @@
     if (!m_serviceWorkerConnection)
         return;
 
-    auto controllingServiceWorkerIdentifier = activeServiceWorker() ? std::make_optional<ServiceWorkerIdentifier>(activeServiceWorker()->identifier()) : std::nullopt;
-    m_serviceWorkerConnection->registerServiceWorkerClient(topOrigin(), ServiceWorkerClientData::from(*this, *serviceWorkerConnection), controllingServiceWorkerIdentifier);
+    auto controllingServiceWorkerRegistrationIdentifier = activeServiceWorker() ? std::make_optional<ServiceWorkerRegistrationIdentifier>(activeServiceWorker()->registrationIdentifier()) : std::nullopt;
+    m_serviceWorkerConnection->registerServiceWorkerClient(topOrigin(), ServiceWorkerClientData::from(*this, *serviceWorkerConnection), controllingServiceWorkerRegistrationIdentifier);
 }
 #endif
 

Modified: trunk/Source/WebCore/workers/service/SWClientConnection.h (229182 => 229183)


--- trunk/Source/WebCore/workers/service/SWClientConnection.h	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/workers/service/SWClientConnection.h	2018-03-02 19:17:02 UTC (rev 229183)
@@ -80,7 +80,7 @@
     virtual bool mayHaveServiceWorkerRegisteredForOrigin(const SecurityOrigin&) const = 0;
     virtual void syncTerminateWorker(ServiceWorkerIdentifier) = 0;
 
-    virtual void registerServiceWorkerClient(const SecurityOrigin& topOrigin, const ServiceWorkerClientData&, const std::optional<ServiceWorkerIdentifier>&) = 0;
+    virtual void registerServiceWorkerClient(const SecurityOrigin& topOrigin, const ServiceWorkerClientData&, const std::optional<ServiceWorkerRegistrationIdentifier>&) = 0;
     virtual void unregisterServiceWorkerClient(DocumentIdentifier) = 0;
 
     virtual void finishFetchingScriptInServer(const ServiceWorkerFetchResult&) = 0;

Modified: trunk/Source/WebCore/workers/service/server/SWServer.cpp (229182 => 229183)


--- trunk/Source/WebCore/workers/service/server/SWServer.cpp	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/workers/service/server/SWServer.cpp	2018-03-02 19:17:02 UTC (rev 229183)
@@ -417,8 +417,13 @@
 
     Vector<ServiceWorkerClientData> matchingClients;
     forEachClientForOrigin(worker.origin(), [&](auto& clientData) {
-        if (!options.includeUncontrolled && worker.identifier() != m_clientToControllingWorker.get(clientData.identifier))
-            return;
+        if (!options.includeUncontrolled) {
+            auto registrationIdentifier = m_clientToControllingRegistration.get(clientData.identifier);
+            if (worker.data().registrationIdentifier != registrationIdentifier)
+                return;
+            if (&worker != activeWorkerFromRegistrationID(registrationIdentifier))
+                return;
+        }
         if (options.type != ServiceWorkerClientType::All && options.type != clientData.type)
             return;
         matchingClients.append(clientData);
@@ -447,13 +452,13 @@
         if (!(registration && registration->key() == worker.registrationKey()))
             return;
 
-        auto result = m_clientToControllingWorker.add(clientData.identifier, worker.identifier());
+        auto result = m_clientToControllingRegistration.add(clientData.identifier, registration->identifier());
         if (!result.isNewEntry) {
             auto previousIdentifier = result.iterator->value;
-            if (previousIdentifier == worker.identifier())
+            if (previousIdentifier == registration->identifier())
                 return;
-            result.iterator->value = worker.identifier();
-            if (auto* controllingRegistration = this->registrationFromServiceWorkerIdentifier(previousIdentifier))
+            result.iterator->value = registration->identifier();
+            if (auto* controllingRegistration = m_registrationsByID.get(previousIdentifier))
                 controllingRegistration->removeClientUsingRegistration(clientData.identifier);
         }
         registration->controlClient(clientData.identifier);
@@ -726,11 +731,6 @@
     return (selectedRegistration && !selectedRegistration->isUninstalling()) ? selectedRegistration : nullptr;
 }
 
-void SWServer::setClientActiveWorker(ServiceWorkerClientIdentifier clientIdentifier, ServiceWorkerIdentifier serviceWorkerIdentifier)
-{
-    m_clientToControllingWorker.set(clientIdentifier, serviceWorkerIdentifier);
-}
-
 SWServerRegistration* SWServer::registrationFromServiceWorkerIdentifier(ServiceWorkerIdentifier identifier)
 {
     auto iterator = m_runningOrTerminatingWorkers.find(identifier);
@@ -740,7 +740,7 @@
     return m_registrations.get(iterator->value->registrationKey());
 }
 
-void SWServer::registerServiceWorkerClient(ClientOrigin&& clientOrigin, ServiceWorkerClientData&& data, const std::optional<ServiceWorkerIdentifier>& controllingServiceWorkerIdentifier)
+void SWServer::registerServiceWorkerClient(ClientOrigin&& clientOrigin, ServiceWorkerClientData&& data, const std::optional<ServiceWorkerRegistrationIdentifier>& controllingServiceWorkerRegistrationIdentifier)
 {
     auto clientIdentifier = data.identifier;
     auto addResult = m_clientsById.add(clientIdentifier, WTFMove(data));
@@ -752,14 +752,16 @@
     clientIdentifiersForOrigin.identifiers.append(clientIdentifier);
     clientIdentifiersForOrigin.terminateServiceWorkersTimer = nullptr;
 
-    if (!controllingServiceWorkerIdentifier)
+    if (!controllingServiceWorkerRegistrationIdentifier)
         return;
 
-    if (auto* controllingRegistration = registrationFromServiceWorkerIdentifier(*controllingServiceWorkerIdentifier)) {
-        controllingRegistration->addClientUsingRegistration(clientIdentifier);
-        auto result = m_clientToControllingWorker.add(clientIdentifier, *controllingServiceWorkerIdentifier);
-        ASSERT_UNUSED(result, result.isNewEntry);
-    }
+    auto* controllingRegistration = m_registrationsByID.get(*controllingServiceWorkerRegistrationIdentifier);
+    if (!controllingRegistration || !controllingRegistration->activeWorker())
+        return;
+
+    controllingRegistration->addClientUsingRegistration(clientIdentifier);
+    auto result = m_clientToControllingRegistration.add(clientIdentifier, *controllingServiceWorkerRegistrationIdentifier);
+    ASSERT_UNUSED(result, result.isNewEntry);
 }
 
 void SWServer::unregisterServiceWorkerClient(const ClientOrigin& clientOrigin, ServiceWorkerClientIdentifier clientIdentifier)
@@ -786,14 +788,14 @@
         iterator->value.terminateServiceWorkersTimer->startOneShot(terminationDelay);
     }
 
-    auto workerIterator = m_clientToControllingWorker.find(clientIdentifier);
-    if (workerIterator == m_clientToControllingWorker.end())
+    auto registrationIterator = m_clientToControllingRegistration.find(clientIdentifier);
+    if (registrationIterator == m_clientToControllingRegistration.end())
         return;
 
-    if (auto* controllingRegistration = registrationFromServiceWorkerIdentifier(workerIterator->value))
-        controllingRegistration->removeClientUsingRegistration(clientIdentifier);
+    if (auto* registration = m_registrationsByID.get(registrationIterator->value))
+        registration->removeClientUsingRegistration(clientIdentifier);
 
-    m_clientToControllingWorker.remove(workerIterator);
+    m_clientToControllingRegistration.remove(registrationIterator);
 }
 
 void SWServer::resolveRegistrationReadyRequests(SWServerRegistration& registration)

Modified: trunk/Source/WebCore/workers/service/server/SWServer.h (229182 => 229183)


--- trunk/Source/WebCore/workers/service/server/SWServer.h	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/workers/service/server/SWServer.h	2018-03-02 19:17:02 UTC (rev 229183)
@@ -161,13 +161,12 @@
     
     WEBCORE_EXPORT static HashSet<SWServer*>& allServers();
 
-    WEBCORE_EXPORT void registerServiceWorkerClient(ClientOrigin&&, ServiceWorkerClientData&&, const std::optional<ServiceWorkerIdentifier>& controllingServiceWorkerIdentifier);
+    WEBCORE_EXPORT void registerServiceWorkerClient(ClientOrigin&&, ServiceWorkerClientData&&, const std::optional<ServiceWorkerRegistrationIdentifier>&);
     WEBCORE_EXPORT void unregisterServiceWorkerClient(const ClientOrigin&, ServiceWorkerClientIdentifier);
 
     using RunServiceWorkerCallback = WTF::Function<void(bool, SWServerToContextConnection&)>;
     WEBCORE_EXPORT void runServiceWorkerIfNecessary(ServiceWorkerIdentifier, RunServiceWorkerCallback&&);
 
-    void setClientActiveWorker(ServiceWorkerClientIdentifier, ServiceWorkerIdentifier);
     void resolveRegistrationReadyRequests(SWServerRegistration&);
 
     void addRegistrationFromStore(ServiceWorkerContextData&&);
@@ -221,7 +220,7 @@
     };
     HashMap<ClientOrigin, Clients> m_clientIdentifiersPerOrigin;
     HashMap<ServiceWorkerClientIdentifier, ServiceWorkerClientData> m_clientsById;
-    HashMap<ServiceWorkerClientIdentifier, ServiceWorkerIdentifier> m_clientToControllingWorker;
+    HashMap<ServiceWorkerClientIdentifier, ServiceWorkerRegistrationIdentifier> m_clientToControllingRegistration;
 
     UniqueRef<SWOriginStore> m_originStore;
     RegistrationStore m_registrationStore;

Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp (229182 => 229183)


--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp	2018-03-02 19:17:02 UTC (rev 229183)
@@ -307,10 +307,7 @@
 
     // For each service worker client who is using registration:
     // - Set client's active worker to registration's active worker.
-    for (auto keyValue : m_clientsUsingRegistration) {
-        for (auto& clientIdentifier : keyValue.value)
-            m_server.setClientActiveWorker(ServiceWorkerClientIdentifier { keyValue.key, clientIdentifier }, activeWorker()->identifier());
-    }
+
     // - Invoke Notify Controller Change algorithm with client as the argument.
     notifyClientsOfControllerChange();
 

Modified: trunk/Source/WebKit/ChangeLog (229182 => 229183)


--- trunk/Source/WebKit/ChangeLog	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/ChangeLog	2018-03-02 19:17:02 UTC (rev 229183)
@@ -1,3 +1,21 @@
+2018-03-02  Youenn Fablet  <you...@apple.com>
+
+        Clients should register to StorageProcess with their service worker registration identifier
+        https://bugs.webkit.org/show_bug.cgi?id=182313
+        <rdar://problem/38044403>
+
+        Reviewed by Chris Dumez.
+
+        Relanding.
+
+        * StorageProcess/ServiceWorker/WebSWServerConnection.cpp:
+        (WebKit::WebSWServerConnection::registerServiceWorkerClient):
+        * StorageProcess/ServiceWorker/WebSWServerConnection.h:
+        * StorageProcess/ServiceWorker/WebSWServerConnection.messages.in:
+        * WebProcess/Storage/WebSWClientConnection.cpp:
+        (WebKit::WebSWClientConnection::registerServiceWorkerClient):
+        * WebProcess/Storage/WebSWClientConnection.h:
+
 2018-03-02  Tim Horton  <timothy_hor...@apple.com>
 
         Make it possible to disable WKPDFView

Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp (229182 => 229183)


--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp	2018-03-02 19:17:02 UTC (rev 229183)
@@ -278,11 +278,11 @@
     send(Messages::WebSWClientConnection::DidGetRegistrations { registrationMatchRequestIdentifier, registrations });
 }
 
-void WebSWServerConnection::registerServiceWorkerClient(SecurityOriginData&& topOrigin, ServiceWorkerClientData&& data, const std::optional<ServiceWorkerIdentifier>& controllingServiceWorkerIdentifier)
+void WebSWServerConnection::registerServiceWorkerClient(SecurityOriginData&& topOrigin, ServiceWorkerClientData&& data, const std::optional<ServiceWorkerRegistrationIdentifier>& controllingServiceWorkerRegistrationIdentifier)
 {
     auto clientOrigin = ClientOrigin { WTFMove(topOrigin), SecurityOriginData::fromSecurityOrigin(SecurityOrigin::create(data.url)) };
     m_clientOrigins.add(data.identifier, clientOrigin);
-    server().registerServiceWorkerClient(WTFMove(clientOrigin), WTFMove(data), controllingServiceWorkerIdentifier);
+    server().registerServiceWorkerClient(WTFMove(clientOrigin), WTFMove(data), controllingServiceWorkerRegistrationIdentifier);
 }
 
 void WebSWServerConnection::unregisterServiceWorkerClient(const ServiceWorkerClientIdentifier& clientIdentifier)

Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h (229182 => 229183)


--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h	2018-03-02 19:17:02 UTC (rev 229183)
@@ -92,7 +92,7 @@
     void matchRegistration(uint64_t registrationMatchRequestIdentifier, const WebCore::SecurityOriginData& topOrigin, const WebCore::URL& clientURL);
     void getRegistrations(uint64_t registrationMatchRequestIdentifier, const WebCore::SecurityOriginData& topOrigin, const WebCore::URL& clientURL);
 
-    void registerServiceWorkerClient(WebCore::SecurityOriginData&& topOrigin, WebCore::ServiceWorkerClientData&&, const std::optional<WebCore::ServiceWorkerIdentifier>&);
+    void registerServiceWorkerClient(WebCore::SecurityOriginData&& topOrigin, WebCore::ServiceWorkerClientData&&, const std::optional<WebCore::ServiceWorkerRegistrationIdentifier>&);
     void unregisterServiceWorkerClient(const WebCore::ServiceWorkerClientIdentifier&);
 
     IPC::Connection* messageSenderConnection() final { return m_contentConnection.ptr(); }

Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in (229182 => 229183)


--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in	2018-03-02 19:17:02 UTC (rev 229183)
@@ -38,7 +38,7 @@
     MatchRegistration(uint64_t serviceRegistrationMatchRequestIdentifier, struct WebCore::SecurityOriginData topOrigin, WebCore::URL clientURL)
     WhenRegistrationReady(uint64_t serviceRegistrationMatchRequestIdentifier, struct WebCore::SecurityOriginData topOrigin, WebCore::URL clientURL)
     GetRegistrations(uint64_t serviceRegistrationMatchRequestIdentifier, struct WebCore::SecurityOriginData topOrigin, WebCore::URL clientURL)
-    RegisterServiceWorkerClient(struct WebCore::SecurityOriginData topOrigin, struct WebCore::ServiceWorkerClientData data, std::optional<WebCore::ServiceWorkerIdentifier> controllingServiceWorkerIdentifier)
+    RegisterServiceWorkerClient(struct WebCore::SecurityOriginData topOrigin, struct WebCore::ServiceWorkerClientData data, std::optional<WebCore::ServiceWorkerRegistrationIdentifier> controllingServiceWorkerRegistrationIdentifier)
     UnregisterServiceWorkerClient(struct WebCore::ServiceWorkerClientIdentifier identifier)
 
     SyncTerminateWorker(WebCore::ServiceWorkerIdentifier workerIdentifier) -> ()

Modified: trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp (229182 => 229183)


--- trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp	2018-03-02 19:17:02 UTC (rev 229183)
@@ -93,9 +93,9 @@
     WebProcess::singleton().send(Messages::WebProcessPool::PostMessageToServiceWorker(destinationIdentifier, WTFMove(message), sourceIdentifier, serverConnectionIdentifier()), 0);
 }
 
-void WebSWClientConnection::registerServiceWorkerClient(const SecurityOrigin& topOrigin, const WebCore::ServiceWorkerClientData& data, const std::optional<WebCore::ServiceWorkerIdentifier>& controllingServiceWorkerIdentifier)
+void WebSWClientConnection::registerServiceWorkerClient(const SecurityOrigin& topOrigin, const WebCore::ServiceWorkerClientData& data, const std::optional<WebCore::ServiceWorkerRegistrationIdentifier>& controllingServiceWorkerRegistrationIdentifier)
 {
-    send(Messages::WebSWServerConnection::RegisterServiceWorkerClient { SecurityOriginData::fromSecurityOrigin(topOrigin), data, controllingServiceWorkerIdentifier });
+    send(Messages::WebSWServerConnection::RegisterServiceWorkerClient { SecurityOriginData::fromSecurityOrigin(topOrigin), data, controllingServiceWorkerRegistrationIdentifier });
 }
 
 void WebSWClientConnection::unregisterServiceWorkerClient(DocumentIdentifier contextIdentifier)

Modified: trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h (229182 => 229183)


--- trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h	2018-03-02 18:33:54 UTC (rev 229182)
+++ trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h	2018-03-02 19:17:02 UTC (rev 229183)
@@ -75,7 +75,7 @@
     void scheduleJobInServer(const WebCore::ServiceWorkerJobData&) final;
     void finishFetchingScriptInServer(const WebCore::ServiceWorkerFetchResult&) final;
     void postMessageToServiceWorker(WebCore::ServiceWorkerIdentifier destinationIdentifier, WebCore::MessageWithMessagePorts&&, const WebCore::ServiceWorkerOrClientIdentifier& source) final;
-    void registerServiceWorkerClient(const WebCore::SecurityOrigin& topOrigin, const WebCore::ServiceWorkerClientData&, const std::optional<WebCore::ServiceWorkerIdentifier>&) final;
+    void registerServiceWorkerClient(const WebCore::SecurityOrigin& topOrigin, const WebCore::ServiceWorkerClientData&, const std::optional<WebCore::ServiceWorkerRegistrationIdentifier>&) final;
     void unregisterServiceWorkerClient(WebCore::DocumentIdentifier) final;
 
     void matchRegistration(const WebCore::SecurityOrigin& topOrigin, const WebCore::URL& clientURL, RegistrationCallback&&) final;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to