Bug 179480 – matchRegistration does not need to go to StorageProcess if no service worker is registered

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

This is a small optimization

[youenn fablet]

Created attachment 326451 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[youenn fablet]

Created attachment 326505 [details]
Patch

[Chris Dumez]

Comment on attachment 326505 [details]
Patch

The new test results look like regression to me. I think we need to investigate them.

[youenn fablet]

Created attachment 326516 [details]
Patch

[youenn fablet]

Created attachment 326745 [details]
Patch

[youenn fablet]

Created attachment 326765 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 326786 [details]
Patch

[youenn fablet]

> 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.

[Chris Dumez]

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?

[Chris Dumez]

(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?

[youenn fablet]

Created attachment 326805 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

Created attachment 326806 [details]
Patch

[Chris Dumez]

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;
});

[youenn fablet]

Created attachment 326813 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326813 [details]
Patch for landing

Clearing flags on attachment: 326813

Committed r224792: <https://trac.webkit.org/changeset/224792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

W3C PR is at https://github.com/w3c/web-platform-tests/pull/8177

[Radar WebKit Bug Importer]

<rdar://problem/35562119>

[Chris Dumez]

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.

[youenn fablet]

(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

[Chris Dumez]

(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.