Bug 180167 - Implement https://w3c.github.io/ServiceWorker/#clients-get
Summary: Implement https://w3c.github.io/ServiceWorker/#clients-get
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-29 14:11 PST by youenn fablet
Modified: 2017-12-01 16:01 PST (History)
10 users (show)

See Also:


Attachments
Patch (35.59 KB, patch)
2017-11-29 17:06 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.48 KB, patch)
2017-11-29 18:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (34.88 KB, patch)
2017-11-30 10:19 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (34.88 KB, patch)
2017-11-30 10:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (34.88 KB, patch)
2017-11-30 10:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (34.88 KB, patch)
2017-11-30 12:15 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.98 KB, patch)
2017-12-01 09:29 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.98 KB, patch)
2017-12-01 10:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.76 KB, patch)
2017-12-01 12:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (37.18 KB, patch)
2017-12-01 14:45 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-11-29 14:11:06 PST
https://w3c.github.io/ServiceWorker/#clients-get
Comment 1 youenn fablet 2017-11-29 17:06:22 PST
Created attachment 327930 [details]
Patch
Comment 2 youenn fablet 2017-11-29 18:09:09 PST
Created attachment 327941 [details]
Patch
Comment 3 youenn fablet 2017-11-30 10:19:52 PST
Created attachment 327988 [details]
Patch
Comment 4 youenn fablet 2017-11-30 10:20:09 PST
Created attachment 327989 [details]
Patch
Comment 5 youenn fablet 2017-11-30 10:22:34 PST
Created attachment 327990 [details]
Patch
Comment 6 youenn fablet 2017-11-30 12:15:05 PST
Created attachment 328004 [details]
Patch
Comment 7 Chris Dumez 2017-11-30 14:52:12 PST
Comment on attachment 328004 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61
> +        if (!identifier)

We should probably check that the identifier is not 0 either.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:32
> +#include "JSServiceWorkerWindowClient.h"

I always find it fishy when we include bindings files in WebCore implementation objects.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:73
> +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value)

I think the logic belongs in ExceptionOr.h. Something like:
#include <wtf/CrossThreadCopier.h>
template<typename T> struct CrossThreadCopierBase<false, false, ExceptionOr<T>> {
    typedef ExceptionOr<T> Type;
    static Type copy(const Type& source)
    {
        if (value.hasException())
            return value.exception().isolatedCopy();
        return CrossThreadCopier<T>::copy(value.releaseReturnValue());
    }
};

Then you can use crossThreadCopy() with your type.

> Source/WebCore/workers/service/ServiceWorkerClients.h:43
> +class ServiceWorkerClients : public ThreadSafeRefCounted<ServiceWorkerClients>, public ActiveDOMObject {

Why is this needed? Do we really ref/deref ServiceWorkerClients from the main thread? It does not look to me that you are (or that you should).

> Source/WebCore/workers/service/server/SWServer.cpp:308
> +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier)

ById?

> Source/WebCore/workers/service/server/SWServer.cpp:310
> +    auto clients = m_clients.get(origin);

Why are you copying the vector here? Shouldn't it be auto&?

> Source/WebCore/workers/service/server/SWServerWorker.h:84
> +    const ClientOrigin& origin() const;

I think it weird that a method called SWServerWorker::origin() returns a ClientOrigin type. This is nothing to do with clients per say.
Comment 8 Chris Dumez 2017-11-30 15:00:17 PST
Comment on attachment 328004 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:54
> +inline std::optional<ServiceWorkerClientIdentifier> ServiceWorkerClientIdentifier::fromString(const String& string)

Should probably take a StringView in parameter.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:53
> +void ServiceWorkerClients::getCompleted(ServiceWorkerClientIdentifier identifier, ExceptionOr<std::optional<ServiceWorkerClientData>>&& clientData, DeferredPromise& promiseReference)

I find "getCompleted" naming a bit confusing. something like didFinishGetRequest() would be much clearer to me.
Comment 9 youenn fablet 2017-11-30 16:46:52 PST
(In reply to Chris Dumez from comment #7)
> Comment on attachment 328004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328004&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61
> > +        if (!identifier)
> 
> We should probably check that the identifier is not 0 either.

I am not sure, since the default identifier is 0-0.
I can add an additional check in ServiceWorkerClients to check for that and return early but I am not sure this is worth it.

> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:32
> > +#include "JSServiceWorkerWindowClient.h"
> 
> I always find it fishy when we include bindings files in WebCore
> implementation objects.

We need toJS() for resolving promises.

> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:73
> > +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value)
> 
> I think the logic belongs in ExceptionOr.h. Something like:
> #include <wtf/CrossThreadCopier.h>
> template<typename T> struct CrossThreadCopierBase<false, false,
> ExceptionOr<T>> {
>     typedef ExceptionOr<T> Type;
>     static Type copy(const Type& source)
>     {
>         if (value.hasException())
>             return value.exception().isolatedCopy();
>         return CrossThreadCopier<T>::copy(value.releaseReturnValue());
>     }
> };
> 
> Then you can use crossThreadCopy() with your type.

Will give it a try and will see whether optional is making things more difficult or not.

> > Source/WebCore/workers/service/ServiceWorkerClients.h:43
> > +class ServiceWorkerClients : public ThreadSafeRefCounted<ServiceWorkerClients>, public ActiveDOMObject {
> 
> Why is this needed? Do we really ref/deref ServiceWorkerClients from the
> main thread? It does not look to me that you are (or that you should).

I will rework the patch and remove this change by eliminating any possibility for it to be derefed on the main thread.

> > Source/WebCore/workers/service/server/SWServer.cpp:308
> > +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier)
> 
> ById?

I prefer FromId since a query is being done.

> > Source/WebCore/workers/service/server/SWServer.cpp:310
> > +    auto clients = m_clients.get(origin);
> 
> Why are you copying the vector here? Shouldn't it be auto&?

Will change it.

> > Source/WebCore/workers/service/server/SWServerWorker.h:84
> > +    const ClientOrigin& origin() const;
> 
> I think it weird that a method called SWServerWorker::origin() returns a
> ClientOrigin type. This is nothing to do with clients per say.

SWServerWorker origin is the origin of the client it was spawned from.
Maybe ClientOrigin is not a very good name though since it has more to do with ITP than service worker clients.
Comment 10 Chris Dumez 2017-11-30 16:50:24 PST
Comment on attachment 328004 [details]
Patch

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

>>> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:61
>>> +        if (!identifier)
>> 
>> We should probably check that the identifier is not 0 either.
> 
> I am not sure, since the default identifier is 0-0.
> I can add an additional check in ServiceWorkerClients to check for that and return early but I am not sure this is worth it.

If JavaScript gives you 0-0 and you let it though. Then our code will handle to handle invalid identifiers (0 is invalid). Things like looking up the identifier in a HashMap would likely crash (because 0 is the magic empty value is such HashMap).
Comment 11 Chris Dumez 2017-11-30 16:53:44 PST
Comment on attachment 328004 [details]
Patch

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

>>> Source/WebCore/workers/service/ServiceWorkerClients.cpp:73
>>> +static inline ExceptionOr<std::optional<ServiceWorkerClientData>> isolatedExceptionOrClientData(const ExceptionOr<std::optional<ServiceWorkerClientData>>& value)
>> 
>> I think the logic belongs in ExceptionOr.h. Something like:
>> #include <wtf/CrossThreadCopier.h>
>> template<typename T> struct CrossThreadCopierBase<false, false, ExceptionOr<T>> {
>>     typedef ExceptionOr<T> Type;
>>     static Type copy(const Type& source)
>>     {
>>         if (value.hasException())
>>             return value.exception().isolatedCopy();
>>         return CrossThreadCopier<T>::copy(value.releaseReturnValue());
>>     }
>> };
>> 
>> Then you can use crossThreadCopy() with your type.
> 
> Will give it a try and will see whether optional is making things more difficult or not.

I added support for using crossThreadCopy() with std::optional earlier today :)

>>> Source/WebCore/workers/service/server/SWServer.cpp:308
>>> +std::optional<ServiceWorkerClientData> SWServer::clientFromId(const ClientOrigin& origin, ServiceWorkerClientIdentifier clientIdentifier)
>> 
>> ById?
> 
> I prefer FromId since a query is being done.

This kind of operation is usually a getById though. See for e.g. getElementById(), not getElementFromId().

>>> Source/WebCore/workers/service/server/SWServerWorker.h:84
>>> +    const ClientOrigin& origin() const;
>> 
>> I think it weird that a method called SWServerWorker::origin() returns a ClientOrigin type. This is nothing to do with clients per say.
> 
> SWServerWorker origin is the origin of the client it was spawned from.
> Maybe ClientOrigin is not a very good name though since it has more to do with ITP than service worker clients.

Then call it clientOrigin() ?
Comment 12 youenn fablet 2017-12-01 09:29:48 PST
Created attachment 328102 [details]
Patch
Comment 13 Chris Dumez 2017-12-01 09:44:11 PST
GTK is red.
Comment 14 youenn fablet 2017-12-01 10:09:48 PST
Created attachment 328112 [details]
Patch
Comment 15 youenn fablet 2017-12-01 10:10:37 PST
(In reply to youenn fablet from comment #14)
> Created attachment 328112 [details]
> Patch

build failure log is not clear at all. Let's retry once, it will probably fail but who knows.
Comment 16 youenn fablet 2017-12-01 10:41:07 PST
> build failure log is not clear at all. Let's retry once, it will probably
> fail but who knows.

Was a flaky build issue apparently.
Comment 17 Chris Dumez 2017-12-01 11:53:35 PST
Comment on attachment 328112 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:45
> +    bool isValid() const { return serverConnectionIdentifier.toUInt64() && contextIdentifier.toUInt64(); }

I do not think this is a good. We just never construct invalid identifiers.

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:63
> +        if (!identifier)

I think we should also return std::nullopt if *identifier is 0.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:110
> +                if (state->m_clients)

How can clients ever be null here. If postTaskToServiceWorker managed to post the task, then the ServiceWorker exists, which means we have a ServiceWorkerGlobalScope, which means clients is alive.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:136
> +        request.state->m_clients = nullptr;

It looks fragile that we have to do this.

> Source/WebCore/workers/service/ServiceWorkerClients.h:74
> +    class RequestState : public ThreadSafeRefCounted<RequestState> {

Does this really need to be ThreadSafeRefCounted<RequestState>? Why cannot we pass an identifier to the lambda and then look up the state from m_pendingRequests when we're back on the service worker thread?

> Source/WebCore/workers/service/ServiceWorkerClients.h:76
> +        RequestState(ServiceWorkerClients*, DeferredPromise*, ServiceWorkerIdentifier);

We should not have a public constructor for a RefCounted class. The constructor should be private and there should be a public create() factory function.

> Source/WebCore/workers/service/ServiceWorkerClients.h:78
> +        ServiceWorkerClients* m_clients;

Having public m_* members is not a nice pattern or common practice.

> Source/WebCore/workers/service/ServiceWorkerClients.h:80
> +        ServiceWorkerIdentifier m_identifier;

This name is too generic and confusing because we are also dealing with client identifiers. We should probably call it serviceWorkerIdentifier.
Comment 18 youenn fablet 2017-12-01 12:50:43 PST
Created attachment 328138 [details]
Patch
Comment 19 youenn fablet 2017-12-01 12:54:31 PST
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328112&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:45
> > +    bool isValid() const { return serverConnectionIdentifier.toUInt64() && contextIdentifier.toUInt64(); }
> 
> I do not think this is a good. We just never construct invalid identifiers.

Default constructed identifier is 0-0.
I would guess IPC identifiers could end up be invalid as well.
 
> > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:63
> > +        if (!identifier)
> 
> I think we should also return std::nullopt if *identifier is 0.

OK

> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:110
> > +                if (state->m_clients)
> 
> How can clients ever be null here. If postTaskToServiceWorker managed to
> post the task, then the ServiceWorker exists, which means we have a
> ServiceWorkerGlobalScope, which means clients is alive.

I was thinking that the scope could stop owning the clients, which is not the case.
There is no reason that clients should be an ActiveDOMObject either.
Patch updated accordingly
Comment 20 Chris Dumez 2017-12-01 13:02:38 PST
Comment on attachment 328138 [details]
Patch

r=me if the bots happy.
Comment 21 youenn fablet 2017-12-01 14:45:00 PST
Created attachment 328161 [details]
Rebasing
Comment 22 WebKit Commit Bot 2017-12-01 16:00:47 PST
Comment on attachment 328161 [details]
Rebasing

Clearing flags on attachment: 328161

Committed r225427: <https://trac.webkit.org/changeset/225427>
Comment 23 WebKit Commit Bot 2017-12-01 16:00:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2017-12-01 16:01:46 PST
<rdar://problem/35808294>