This is a small optimization
Created attachment 326451 [details] Patch
Comment on attachment 326451 [details] Patch Attachment 326451 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5164498 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/claim-with-redirect.https.html http/tests/workers/service/service-worker-getRegistration.html imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https.html imported/w3c/web-platform-tests/service-workers/service-worker/activation.https.html
Created attachment 326460 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 326451 [details] Patch Attachment 326451 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5164515 New failing tests: http/tests/workers/service/service-worker-getRegistration.html imported/w3c/web-platform-tests/service-workers/service-worker/claim-with-redirect.https.html imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https.html imported/w3c/web-platform-tests/service-workers/service-worker/activation.https.html
Created attachment 326462 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 326451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326451&action=review > Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:111 > + if (!hasServiceWorkerRegisteredForOrigin(SecurityOrigin::create(clientURL))) { I think we currently use topOrigin in the store.
Created attachment 326505 [details] Patch
Comment on attachment 326505 [details] Patch The new test results look like regression to me. I think we need to investigate them.
Created attachment 326516 [details] Patch
Created attachment 326745 [details] Patch
Created attachment 326765 [details] Patch
Comment on attachment 326765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326765&action=review > Source/WebCore/workers/service/server/SWServer.cpp:87 > + if (!m_registrations.set(key, WTFMove(registration)).isNewEntry) How can it not be a new registration? I think this should be an assertion, not an if check. > Source/WebCore/workers/service/server/SWServer.cpp:91 > + ++m_originCounts.ensure(topOrigin->toString(), [&] { Can we move the originCount to the ServiceWorkerOriginStore for better encapsulation? The server should not have to deal with counts. > Source/WebCore/workers/service/server/SWServer.cpp:100 > + if (!m_registrations.remove(key)) I don't think this should happen. Should probably be an assertion instead of an if check. > Source/WebCore/workers/service/server/SWServer.cpp:120 > + m_originStore->clear(); I think moving the originStore to SWServer is indeed a good idea. > LayoutTests/imported/w3c/ChangeLog:8 > + Updating tests as otherwise they would fail trying to access to document.body which would be null. How is this related to your code change? You code change is not supposed to cause web-facing behavior change AFAICT. Therefore, no test update should be needed.
(In reply to Chris Dumez from comment #12) > Comment on attachment 326765 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326765&action=review > > > Source/WebCore/workers/service/server/SWServer.cpp:87 > > + if (!m_registrations.set(key, WTFMove(registration)).isNewEntry) > > How can it not be a new registration? I think this should be an assertion, > not an if check. > > > Source/WebCore/workers/service/server/SWServer.cpp:91 > > + ++m_originCounts.ensure(topOrigin->toString(), [&] { > > Can we move the originCount to the ServiceWorkerOriginStore for better > encapsulation? The server should not have to deal with counts. > > > Source/WebCore/workers/service/server/SWServer.cpp:100 > > + if (!m_registrations.remove(key)) > > I don't think this should happen. Should probably be an assertion instead of > an if check. > > > Source/WebCore/workers/service/server/SWServer.cpp:120 > > + m_originStore->clear(); > > I think moving the originStore to SWServer is indeed a good idea. > > > LayoutTests/imported/w3c/ChangeLog:8 > > + Updating tests as otherwise they would fail trying to access to document.body which would be null. > > How is this related to your code change? You code change is not supposed to > cause web-facing behavior change AFAICT. Therefore, no test update should be > needed. I think there is some race condition between the type document.body is populated and the time the code using document.body is executed. With this additional optimization, we remove an additional IPC call which makes this issue more visible.
Created attachment 326786 [details] Patch
> I think there is some race condition between the type document.body is > populated and the time the code using document.body is executed. With this > additional optimization, we remove an additional IPC call which makes this > issue more visible. I wrapped one of the test with a setTimeout and removed the html body element. The test then works.
Comment on attachment 326786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326786&action=review > Source/WebCore/workers/service/server/SWServer.h:113 > + HashMap<String, uint64_t> m_originCounts; OriginStore was just supposed to be an interface. We want as much of the store complexity/logic as possible out of SWServer. For this reason, I still believe the counts should be handled at WebSWOriginStore level. WebSWOriginStore basically becomes a HashCountedSet, which is fine. > LayoutTests/imported/w3c/ChangeLog:7 > + What's the upstream PR for this change? why is this needed? Where is document.body used? Could not find body being used in activation.https.html, is it used from an imported script or iframe?
(In reply to Chris Dumez from comment #16) > Comment on attachment 326786 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326786&action=review > > > Source/WebCore/workers/service/server/SWServer.h:113 > > + HashMap<String, uint64_t> m_originCounts; > > OriginStore was just supposed to be an interface. We want as much of the > store complexity/logic as possible out of SWServer. For this reason, I still > believe the counts should be handled at WebSWOriginStore level. > WebSWOriginStore basically becomes a HashCountedSet, which is fine. > > > LayoutTests/imported/w3c/ChangeLog:7 > > + > > What's the upstream PR for this change? why is this needed? Where is > document.body used? Could not find body being used in activation.https.html, > is it used from an imported script or iframe? Note that presumably, those tests are passing in Chrome so this may indicate we have a bug?
Created attachment 326805 [details] Patch
There is also the case that WebSWOriginTable::contains return false if its origin table shared memory is not yet initialized. We should probably go to the StorageProcess in that case. Just trying to tweak it locally and it helps fixing one test, not the other. But then it makes basic-register.html fail. I'll fix that as a follow-up.
Created attachment 326806 [details] Patch
Comment on attachment 326806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326806&action=review r=me with changes. > Source/WebCore/workers/service/server/SWServer.cpp:88 > + auto result = m_registrations.set(key, WTFMove(registration)); Should be an add(), not a set(). We should never overwrite. > Source/WebCore/workers/service/server/SWServer.cpp:115 > + m_registrations.removeIf([&] (auto& entry) { We should not make this change unless we also clear the right jobQueues. otherwise, we will end up with jobQueues that are out of sync with the server (expect a registration to be there but isn't). Let's clear the jobQueues: m_jobQueues.removeIf([&] (auto& entry) { return entry.key.topOrigin() == originData; });
Created attachment 326813 [details] Patch for landing
Comment on attachment 326813 [details] Patch for landing Clearing flags on attachment: 326813 Committed r224792: <https://trac.webkit.org/changeset/224792>
All reviewed patches have been landed. Closing bug.
W3C PR is at https://github.com/w3c/web-platform-tests/pull/8177
<rdar://problem/35562119>
Comment on attachment 326813 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=326813&action=review > LayoutTests/imported/w3c/ChangeLog:9 > + * web-platform-tests/service-workers/service-worker/activation.https.html: Is there a PR for this? I could not find it.
(In reply to Chris Dumez from comment #27) > Comment on attachment 326813 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326813&action=review > > > LayoutTests/imported/w3c/ChangeLog:9 > > + * web-platform-tests/service-workers/service-worker/activation.https.html: > > Is there a PR for this? I could not find it. This should be https://github.com/w3c/web-platform-tests/pull/8177
(In reply to youenn fablet from comment #28) > (In reply to Chris Dumez from comment #27) > > Comment on attachment 326813 [details] > > Patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=326813&action=review > > > > > LayoutTests/imported/w3c/ChangeLog:9 > > > + * web-platform-tests/service-workers/service-worker/activation.https.html: > > > > Is there a PR for this? I could not find it. > > This should be https://github.com/w3c/web-platform-tests/pull/8177 I see. I got confused because the body is at a different place upstream and downstream.