Bug 177876 – Add an efficient data structure for WebCore to query if there is a Service Worker registered for a given origin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Introduce ServiceWorkerOriginStore which is a HashSet of origins for which service workers are registered. This HashSet is shared between the StorageProcess and the WebProcesses via shared memory and is used as an optimization to avoid going to the StorageProcess for loads unnecessarily.

[Radar WebKit Bug Importer]

<rdar://problem/34813129>

[Chris Dumez]

Created attachment 322690 [details]
WIP Patch

[Chris Dumez]

Created attachment 322696 [details]
WIP Patch

[Build Bot]

Attachment 322696 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/ServiceWorkerOriginTableController.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebKit/StorageProcess/ServiceWorker/ServiceWorkerOriginStore.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebKit/StorageProcess/StorageProcess.h:131:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/StorageProcess/StorageProcess.h:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 322696 [details]
WIP Patch

Attachment 322696 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4756742

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 322704 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 322710 [details]
WIP Patch

[Chris Dumez]

Created attachment 322711 [details]
WIP Patch

[Chris Dumez]

Created attachment 322714 [details]
WIP Patch

[Chris Dumez]

Brady, is this what you had in mind?

[Chris Dumez]

Created attachment 323562 [details]
WIP Patch

[Chris Dumez]

Created attachment 323600 [details]
WIP Patch

[Chris Dumez]

Created attachment 323602 [details]
WIP Patch

[Chris Dumez]

Created attachment 323693 [details]
WIP Patch

[Chris Dumez]

Created attachment 323695 [details]
Patch

[Chris Dumez]

Created attachment 323696 [details]
Patch

[Chris Dumez]

Created attachment 323973 [details]
WIP Patch

[Chris Dumez]

Created attachment 323980 [details]
WIP Patch

[Build Bot]

Attachment 323980 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/server/SWServerRegistration.h:53:  The parameter name "registrationKey" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 323989 [details]
WIP Patch

[Build Bot]

Attachment 323989 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/server/SWServerRegistration.h:53:  The parameter name "registrationKey" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 323992 [details]
WIP Patch

[Chris Dumez]

Created attachment 323993 [details]
WIP Patch

[youenn fablet]

Comment on attachment 323993 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323993&action=review

> Source/WebCore/workers/service/ServiceWorkerProvider.h:49
> +    virtual bool hasRegisteredServiceWorkerForOrigin(const String& origin) = 0;

Shouldn't it be a SecurityOrigin& in principle?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:99
> +    ASSERT(m_currentJob);

Is this assert needed?

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:78
> +    StorageProcess::singleton().serviceWorkerOriginStore().addServiceWorkerHash(computeSharedStringHash(origin->toString()));

For handling fetch, we will probably end up going with the SW loading path based on this hash set.
There are a number of cases where we will go down that path while we shouldn't have:
- SW not active, SW not handling fetch events, navigation loads, foreign fetch
I wonder whether/how we can extend this store to contain more information/more state.

For this early phase, this is probably fine for SW fetch handle.
We will go to SW and if there is nothing to be done, we will be back to WebProcess to start load through the NetworkProcess.

> Source/WebKit/StorageProcess/StorageProcess.h:154
> +    std::unique_ptr<ServiceWorkerOriginStore> m_serviceWorkerOriginStore;

UniqueRef probably

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:65
> +    return WebProcess::singleton().webToStorageProcessConnection()->serviceWorkerOriginTableController().isServiceWorkerRegisteredForOrigin(computeSharedStringHash(origin));

Shouldn't the table be per session?

> Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.h:87
> +    std::unique_ptr<ServiceWorkerOriginTableController> m_serviceWorkerOriginTableController;

UniqueRef?

[Chris Dumez]

Created attachment 324040 [details]
WIP Patch

[Chris Dumez]

Created attachment 324044 [details]
WIP Patch

[Chris Dumez]

Created attachment 324045 [details]
Patch

[Chris Dumez]

Created attachment 324052 [details]
Patch

[youenn fablet]

Comment on attachment 324052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324052&action=review

> Source/WebCore/workers/service/ServiceWorkerProvider.h:50
> +    virtual bool hasRegisteredServiceWorkerForOrigin(const PAL::SessionID&, const SecurityOrigin&) = 0;

SessionID is a uint64_t, shouldn't we pass it by value?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:99
> +    ASSERT(m_currentJob);

Do we need that assert?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:100
> +    UNUSED_PARAM(workerID);

Can we simply remove workerID?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:102
> +    resolveCurrentJob(ServiceWorkerRegistrationData { registrationKey, identifier });

I guess we could move registrationKey, meaning to pass it as an r-value.

> Source/WebKit/StorageProcess/ServiceWorker/ServiceWorkerOriginStore.h:40
> +    ServiceWorkerOriginStore(StorageProcess&, const PAL::SessionID&);

Ditto for SessionID.

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:78
> +    StorageProcess::singleton().ensureSWOriginStoreForSession(m_sessionID).addServiceWorkerHash(computeSharedStringHash(origin->toString()));

The SW in ensureSWOriginStoreForSession seems a bit odd but I guess this is consistent with other names.

> Source/WebKit/StorageProcess/StorageProcess.cpp:155
> +    for (auto& storageToWebProcessConnection : m_storageToWebProcessConnections)

Shouldn't m_storageToWebProcessConnections take Ref<>?

> Source/WebKit/StorageProcess/StorageProcess.cpp:370
> +    return *m_swOriginStores.ensure(sessionID, [this, &sessionID] {

sessionID by value?

> Source/WebKit/StorageProcess/StorageProcess.cpp:371
> +        return std::make_unique<ServiceWorkerOriginStore>(*this, sessionID);

Is UniqueRef working with HashMap?

> Source/WebKit/StorageProcess/StorageProcess.cpp:414
> +void StorageProcess::serviceWorkerContextStarted(uint64_t serverConnectionIdentifier, const ServiceWorkerRegistrationKey& registrationKey, uint64_t identifier, const String& workerID)

We can probably take a ServiceWorkerRegistrationKey&& and move it below.

> Source/WebKit/WebProcess/Storage/ServiceWorkerOriginTableController.h:52
> +    HashMap<PAL::SessionID, std::unique_ptr<SharedStringHashTable>> m_serviceWorkerOriginTables;

The alternative might have been to put the controller as a child of WebSWClientConnection which is per-session directly.
This would remove the need for this m_serviceWorkerOriginTables map.
SetServiceWorkerOriginTable message would probably be in WebSWClientConnection
I guess this should be mirrored in StorageProcess as well.

[Chris Dumez]

Comment on attachment 324052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324052&action=review

>> Source/WebCore/workers/service/ServiceWorkerProvider.h:50
>> +    virtual bool hasRegisteredServiceWorkerForOrigin(const PAL::SessionID&, const SecurityOrigin&) = 0;
> 
> SessionID is a uint64_t, shouldn't we pass it by value?

Will pass by value.

>> Source/WebCore/workers/service/server/SWServerRegistration.cpp:100
>> +    UNUSED_PARAM(workerID);
> 
> Can we simply remove workerID?

Brady is passing it in the error case so I am passing it in the success case too. My assumption is that we will need it in a follow-up patch.

>> Source/WebKit/StorageProcess/StorageProcess.cpp:371
>> +        return std::make_unique<ServiceWorkerOriginStore>(*this, sessionID);
> 
> Is UniqueRef working with HashMap?

It is but UniqueRef::get() is const-correct so I would've to const_cast in swOriginStoreForSession() :(

>> Source/WebKit/WebProcess/Storage/ServiceWorkerOriginTableController.h:52
>> +    HashMap<PAL::SessionID, std::unique_ptr<SharedStringHashTable>> m_serviceWorkerOriginTables;
> 
> The alternative might have been to put the controller as a child of WebSWClientConnection which is per-session directly.
> This would remove the need for this m_serviceWorkerOriginTables map.
> SetServiceWorkerOriginTable message would probably be in WebSWClientConnection
> I guess this should be mirrored in StorageProcess as well.

Will look into this.

[youenn fablet]

> >> Source/WebCore/workers/service/server/SWServerRegistration.cpp:100
> >> +    UNUSED_PARAM(workerID);
> > 
> > Can we simply remove workerID?
> 
> Brady is passing it in the error case so I am passing it in the success case
> too. My assumption is that we will need it in a follow-up patch.

I was meaning replacing "const String& workerID" by "const String&" to remove the need for the UNUSED_PARAM.

[Chris Dumez]

(In reply to youenn fablet from comment #31)
> > >> Source/WebCore/workers/service/server/SWServerRegistration.cpp:100
> > >> +    UNUSED_PARAM(workerID);
> > > 
> > > Can we simply remove workerID?
> > 
> > Brady is passing it in the error case so I am passing it in the success case
> > too. My assumption is that we will need it in a follow-up patch.
> 
> I was meaning replacing "const String& workerID" by "const String&" to
> remove the need for the UNUSED_PARAM.

I think the parameter name is actually useful here. I could use /* workerID */ like we do sometimes but I think UNUSED_PARAM() does not hurt here and it is the pattern used by Brady in the failure method.

[youenn fablet]

Comment on attachment 324052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324052&action=review

> Source/WebCore/workers/service/context/SWContextManager.cpp:67
> +    return generateServiceWorkerIdentifier();

The identifier we return here should probably the key of m_workerThreadMap.
So either we should be returning thread->identifier() or we should use generateServiceWorkerIdentifier().
For now, it seems safer to use generateServiceWorkerIdentifier() since we are sure identifiers will not be reused.

[Chris Dumez]

Created attachment 324086 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 324086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324086&action=review

> Source/WebKit/StorageProcess/StorageProcess.cpp:227
> +        if (auto* store = swOriginStoreForSession(sessionID))
> +            store->clear();

It's a bit scary to have a generic look up function like swOriginStoreForSession since the lifetime of WebSWOriginStore is tied to its unique_ptr,
and there is no guarantee that unique_ptr would outlive the usage of the raw pointer returned by it.

Can we instead add a method to find & call clear or remove data for a set of origins so that we don't have to expose WebSWOriginStore*?

> LayoutTests/ChangeLog:17
> +        (log):
> +        (then):
> +        (catch):

Remove these?

> LayoutTests/ChangeLog:19
> +        (i.then):

Ditto.

[Chris Dumez]

Comment on attachment 324086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324086&action=review

>> Source/WebKit/StorageProcess/StorageProcess.cpp:227
>> +            store->clear();
> 
> It's a bit scary to have a generic look up function like swOriginStoreForSession since the lifetime of WebSWOriginStore is tied to its unique_ptr,
> and there is no guarantee that unique_ptr would outlive the usage of the raw pointer returned by it.
> 
> Can we instead add a method to find & call clear or remove data for a set of origins so that we don't have to expose WebSWOriginStore*?

WebSWOriginStore is our abstraction for these operations. Adding (at least 3) methods related to adding/removing/origins from the store on the StorageProcess class would be poor factoring in my opinion.

Having a "non-ensure" method that returns a raw pointer and an "ensure" method that returns a reference is a common pattern in WebKit. I would agree that developers have to be careful to the lifetime of the WebSWOriginStore object and that we may want to find a better pattern. However, I am not convinced yet that this better pattern is to not expose the store and mirror its methods on the StorageProcess.

[Chris Dumez]

(In reply to Chris Dumez from comment #36)
> Comment on attachment 324086 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324086&action=review
> 
> >> Source/WebKit/StorageProcess/StorageProcess.cpp:227
> >> +            store->clear();
> > 
> > It's a bit scary to have a generic look up function like swOriginStoreForSession since the lifetime of WebSWOriginStore is tied to its unique_ptr,
> > and there is no guarantee that unique_ptr would outlive the usage of the raw pointer returned by it.
> > 
> > Can we instead add a method to find & call clear or remove data for a set of origins so that we don't have to expose WebSWOriginStore*?
> 
> WebSWOriginStore is our abstraction for these operations. Adding (at least
> 3) methods related to adding/removing/origins from the store on the
> StorageProcess class would be poor factoring in my opinion.
> 
> Having a "non-ensure" method that returns a raw pointer and an "ensure"
> method that returns a reference is a common pattern in WebKit. I would agree
> that developers have to be careful to the lifetime of the WebSWOriginStore
> object and that we may want to find a better pattern. However, I am not
> convinced yet that this better pattern is to not expose the store and mirror
> its methods on the StorageProcess.

Will land to unblock Youenn and further work on SW. We can refactor in a follow-up.

[Chris Dumez]

Committed r223608: <https://trac.webkit.org/changeset/223608>

[Matt Lewis]

This change caused the test http/tests/workers/service/basic-register.html to become very flaky:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fbasic-register.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r223632%20(5312)/results.html
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/5312

diff:
--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/basic-register-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/basic-register-actual.txt
@@ -1,7 +1,7 @@
 PASS: No service worker is initially registered for this origin
 PASS: No service worker is initially registered for this origin in private session
 Registered!
-PASS: A service worker is now registered for this origin
+FAIL: No service worker is registered for this origin
 PASS: No service worker is registered for this origin in private session
 Registered!