Bug 180167 – Implement https://w3c.github.io/ServiceWorker/#clients-get

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

[youenn fablet]

https://w3c.github.io/ServiceWorker/#clients-get

[youenn fablet]

Created attachment 327930 [details]
Patch

[youenn fablet]

Created attachment 327941 [details]
Patch

[youenn fablet]

Created attachment 327988 [details]
Patch

[youenn fablet]

Created attachment 327989 [details]
Patch

[youenn fablet]

Created attachment 327990 [details]
Patch

[youenn fablet]

Created attachment 328004 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[youenn fablet]

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

[Chris Dumez]

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

[Chris Dumez]

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

[youenn fablet]

Created attachment 328102 [details]
Patch

[Chris Dumez]

GTK is red.

[youenn fablet]

Created attachment 328112 [details]
Patch

[youenn fablet]

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

[youenn fablet]

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

[Chris Dumez]

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.

[youenn fablet]

Created attachment 328138 [details]
Patch

[youenn fablet]

> 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

[Chris Dumez]

Comment on attachment 328138 [details]
Patch

r=me if the bots happy.

[youenn fablet]

Created attachment 328161 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 328161 [details]
Rebasing

Clearing flags on attachment: 328161

Committed r225427: <https://trac.webkit.org/changeset/225427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35808294>