Bug 177876 - Add an efficient data structure for WebCore to query if there is a Service Worker registered for a given origin
Summary: Add an efficient data structure for WebCore to query if there is a Service Wo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-04 09:28 PDT by Chris Dumez
Modified: 2017-10-18 17:58 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (28.62 KB, patch)
2017-10-04 10:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (29.23 KB, patch)
2017-10-04 11:41 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (968.38 KB, application/zip)
2017-10-04 12:48 PDT, Build Bot
no flags Details
WIP Patch (29.57 KB, patch)
2017-10-04 13:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (28.68 KB, patch)
2017-10-04 13:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (29.86 KB, patch)
2017-10-04 13:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (29.95 KB, patch)
2017-10-12 14:11 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (30.20 KB, patch)
2017-10-12 16:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (30.28 KB, patch)
2017-10-12 16:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (31.40 KB, patch)
2017-10-13 10:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (35.37 KB, patch)
2017-10-13 10:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (35.40 KB, patch)
2017-10-13 10:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (37.89 KB, patch)
2017-10-16 19:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (57.58 KB, patch)
2017-10-16 20:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (63.13 KB, patch)
2017-10-16 21:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (62.81 KB, patch)
2017-10-16 21:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (53.20 KB, patch)
2017-10-16 21:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (56.93 KB, patch)
2017-10-17 12:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (57.81 KB, patch)
2017-10-17 13:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (67.90 KB, patch)
2017-10-17 13:11 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (69.46 KB, patch)
2017-10-17 13:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (68.45 KB, patch)
2017-10-17 19:41 PDT, Chris Dumez
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-10-04 09:28:33 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-10-04 09:29:13 PDT
<rdar://problem/34813129>
Comment 2 Chris Dumez 2017-10-04 10:59:46 PDT
Created attachment 322690 [details]
WIP Patch
Comment 3 Chris Dumez 2017-10-04 11:41:04 PDT
Created attachment 322696 [details]
WIP Patch
Comment 4 Build Bot 2017-10-04 11:42:54 PDT
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.
Comment 5 Build Bot 2017-10-04 12:48:28 PDT
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.
Comment 6 Build Bot 2017-10-04 12:48:30 PDT
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
Comment 7 Chris Dumez 2017-10-04 13:18:48 PDT
Created attachment 322710 [details]
WIP Patch
Comment 8 Chris Dumez 2017-10-04 13:22:45 PDT
Created attachment 322711 [details]
WIP Patch
Comment 9 Chris Dumez 2017-10-04 13:41:58 PDT
Created attachment 322714 [details]
WIP Patch
Comment 10 Chris Dumez 2017-10-04 14:20:11 PDT
Brady, is this what you had in mind?
Comment 11 Chris Dumez 2017-10-12 14:11:17 PDT
Created attachment 323562 [details]
WIP Patch
Comment 12 Chris Dumez 2017-10-12 16:52:39 PDT
Created attachment 323600 [details]
WIP Patch
Comment 13 Chris Dumez 2017-10-12 16:58:58 PDT
Created attachment 323602 [details]
WIP Patch
Comment 14 Chris Dumez 2017-10-13 10:24:34 PDT
Created attachment 323693 [details]
WIP Patch
Comment 15 Chris Dumez 2017-10-13 10:32:16 PDT
Created attachment 323695 [details]
Patch
Comment 16 Chris Dumez 2017-10-13 10:34:14 PDT
Created attachment 323696 [details]
Patch
Comment 17 Chris Dumez 2017-10-16 19:28:27 PDT
Created attachment 323973 [details]
WIP Patch
Comment 18 Chris Dumez 2017-10-16 20:31:25 PDT
Created attachment 323980 [details]
WIP Patch
Comment 19 Build Bot 2017-10-16 20:34:52 PDT
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.
Comment 20 Chris Dumez 2017-10-16 21:30:50 PDT
Created attachment 323989 [details]
WIP Patch
Comment 21 Build Bot 2017-10-16 21:33:32 PDT
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.
Comment 22 Chris Dumez 2017-10-16 21:35:13 PDT
Created attachment 323992 [details]
WIP Patch
Comment 23 Chris Dumez 2017-10-16 21:42:22 PDT
Created attachment 323993 [details]
WIP Patch
Comment 24 youenn fablet 2017-10-17 08:34:48 PDT
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?
Comment 25 Chris Dumez 2017-10-17 12:33:20 PDT
Created attachment 324040 [details]
WIP Patch
Comment 26 Chris Dumez 2017-10-17 13:03:12 PDT
Created attachment 324044 [details]
WIP Patch
Comment 27 Chris Dumez 2017-10-17 13:11:18 PDT
Created attachment 324045 [details]
Patch
Comment 28 Chris Dumez 2017-10-17 13:54:39 PDT
Created attachment 324052 [details]
Patch
Comment 29 youenn fablet 2017-10-17 15:09:44 PDT
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.
Comment 30 Chris Dumez 2017-10-17 15:28:47 PDT
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.
Comment 31 youenn fablet 2017-10-17 15:30:59 PDT
> >> 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.
Comment 32 Chris Dumez 2017-10-17 15:34:20 PDT
(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.
Comment 33 youenn fablet 2017-10-17 17:27:35 PDT
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.
Comment 34 Chris Dumez 2017-10-17 19:41:45 PDT
Created attachment 324086 [details]
Patch
Comment 35 Ryosuke Niwa 2017-10-17 23:06:36 PDT
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.
Comment 36 Chris Dumez 2017-10-18 09:03:40 PDT
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.
Comment 37 Chris Dumez 2017-10-18 09:11:07 PDT
(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.
Comment 38 Chris Dumez 2017-10-18 09:12:17 PDT
Committed r223608: <https://trac.webkit.org/changeset/223608>
Comment 39 Matt Lewis 2017-10-18 17:27:08 PDT
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!