Summary: | Add an efficient data structure for WebCore to query if there is a Service Worker registered for a given origin | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, aestes, beidson, buildbot, ggaren, jlewis3, mkwst, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=174541 https://bugs.webkit.org/show_bug.cgi?id=178494 |
||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-10-04 09:28:33 PDT
Created attachment 322690 [details]
WIP Patch
Created attachment 322696 [details]
WIP Patch
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 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. 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
Created attachment 322710 [details]
WIP Patch
Created attachment 322711 [details]
WIP Patch
Created attachment 322714 [details]
WIP Patch
Brady, is this what you had in mind? Created attachment 323562 [details]
WIP Patch
Created attachment 323600 [details]
WIP Patch
Created attachment 323602 [details]
WIP Patch
Created attachment 323693 [details]
WIP Patch
Created attachment 323695 [details]
Patch
Created attachment 323696 [details]
Patch
Created attachment 323973 [details]
WIP Patch
Created attachment 323980 [details]
WIP Patch
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.
Created attachment 323989 [details]
WIP Patch
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.
Created attachment 323992 [details]
WIP Patch
Created attachment 323993 [details]
WIP Patch
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? Created attachment 324040 [details]
WIP Patch
Created attachment 324044 [details]
WIP Patch
Created attachment 324045 [details]
Patch
Created attachment 324052 [details]
Patch
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 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.
> >> 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.
(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 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. Created attachment 324086 [details]
Patch
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 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. (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. Committed r223608: <https://trac.webkit.org/changeset/223608> 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! |