Bug 180047 – Register Documents as ServiceWorker clients to the StorageProcess

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

[youenn fablet]

Storage Process needs to know potential ServiceWorker clients.
Any document which may interact with a service worker should be registered.

[youenn fablet]

Created attachment 327654 [details]
Patch

[youenn fablet]

Created attachment 327663 [details]
Patch

[youenn fablet]

Created attachment 327664 [details]
Patch

[youenn fablet]

Created attachment 327667 [details]
Patch

[EWS Watchlist]

Comment on attachment 327667 [details]
Patch

Attachment 327667 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5378744

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 327675 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 327667 [details]
Patch

Attachment 327667 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5378697

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 327677 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 327667 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 327667 [details]
Patch

Attachment 327667 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5379142

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 327682 [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

[youenn fablet]

Created attachment 327699 [details]
Patch

[youenn fablet]

Created attachment 327711 [details]
Patch

[youenn fablet]

Created attachment 327775 [details]
Patch

[Chris Dumez]

Comment on attachment 327775 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Storing all clients in SWServer as a HasMap keyed by ClientOrigin.

typo: HasMap

> Source/WebCore/dom/Document.cpp:7589
> +void Document::setServiceWorkerConnection(SWClientConnection* serviceWorkerConnection)

This seems to be only called when a service worker starts controlling a document. This seems wrong no? In the current state, it seems identical to the "client using registration" concept which we already have.

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:40
> +    bool operator==(const ServiceWorkerClientIdentifier& identifier) const { return serverConnectionIdentifier == identifier.serverConnectionIdentifier &&  contextIdentifier == identifier.contextIdentifier; }

Darin told me we normally prefer when the operator is outside the class, not a member.

> Source/WebCore/workers/service/server/SWServer.cpp:502
> +    clients.removeFirstMatching([&] (const auto& item) {

Why aren't we using a HashSet?

> Source/WebCore/workers/service/server/SWServer.h:177
> +    HashMap<ClientOrigin, Vector<ClientInformation>> m_clients;

Why are clients stored like this and not on SWServiceWorker? Those are *service worker* clients, right? If so, shouldn't they be tried to a SWServiceWorker object instead of globally on the server?

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:99
> +    HashMap<WebCore::DocumentIdentifier, WebCore::ClientOrigin> m_clientOrigins;

Why do we need another copy of this map? This is basically the same map as on SWServer, no?

[Chris Dumez]

Basically, I expected something like:
HashSet<ServiceWorkerClientIdentifier> m_clients;

on SWServerWorker.

Any reason we cannot / should not do this?

[youenn fablet]

Comment on attachment 327775 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7589
>> +void Document::setServiceWorkerConnection(SWClientConnection* serviceWorkerConnection)
> 
> This seems to be only called when a service worker starts controlling a document. This seems wrong no? In the current state, it seems identical to the "client using registration" concept which we already have.

If service worker runtime flag is on, it will be called for every document in DocumentLoader::commitData.
This patch does the registration as a client just before selecting a service worker if there is one.

>> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:40
>> +    bool operator==(const ServiceWorkerClientIdentifier& identifier) const { return serverConnectionIdentifier == identifier.serverConnectionIdentifier &&  contextIdentifier == identifier.contextIdentifier; }
> 
> Darin told me we normally prefer when the operator is outside the class, not a member.

Right.

>> Source/WebCore/workers/service/server/SWServer.cpp:502
>> +    clients.removeFirstMatching([&] (const auto& item) {
> 
> Why aren't we using a HashSet?

Was not too sure about it.
I picked a Vector since algos like Clients.matchAll are iterating through this list and are generating arrays of clients.
That said, matchAll is requiring some sorting at the end of the algorithm so maybe a HashSet would be better.

>> Source/WebCore/workers/service/server/SWServer.h:177
>> +    HashMap<ClientOrigin, Vector<ClientInformation>> m_clients;
> 
> Why are clients stored like this and not on SWServiceWorker? Those are *service worker* clients, right? If so, shouldn't they be tried to a SWServiceWorker object instead of globally on the server?

Not sure to follow here.
They are indeed service worker clients and any service worker from that ClientOrigin can talk to any of this client.

>> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:99
>> +    HashMap<WebCore::DocumentIdentifier, WebCore::ClientOrigin> m_clientOrigins;
> 
> Why do we need another copy of this map? This is basically the same map as on SWServer, no?

This is a map specific to the clients of a WebProcess, SWServer map is global to all web processes.
m_clientOrigins for instance allows removing all clients of a given WebProcess if it crashes for instance.

[Brady Eidson]

Comment on attachment 327775 [details]
Patch

Little details pointed out by Chris aside, I think this is the necessary approach

[Chris Dumez]

(In reply to Brady Eidson from comment #19)
> Comment on attachment 327775 [details]
> Patch
> 
> Little details pointed out by Chris aside, I think this is the necessary
> approach

Yes, after discussing this offline with Youenn. My initial understanding was wrong and I am ok with this approach as well now.

[youenn fablet]

Created attachment 327807 [details]
Patch for landing

[youenn fablet]

I kept a vector instead of a HashSet for now.
I will revisit this when fully implementing Clients API.

[WebKit Commit Bot]

Comment on attachment 327807 [details]
Patch for landing

Clearing flags on attachment: 327807

Committed r225251: <https://trac.webkit.org/changeset/225251>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35738152>

[youenn fablet]

(In reply to youenn fablet from comment #22)
> I kept a vector instead of a HashSet for now.
> I will revisit this when fully implementing Clients API.

Looking at it further, matched clients should be sorted by their creation order.
This creation order is now supported by Vector.
If we need a more explicit creation order, we should use a HashSet.