Bug 238400 – Support ServiceWorkerClients.openWindow

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

[Brady Eidson]

Support ServiceWorkerClients.openWindow

rdar://90616651

[Brady Eidson]

This is basically a huge plumbing task getting the message up to the client.

And tests for it.

[Brady Eidson]

Created attachment 455811 [details]
EWS v1

Will get a changelog up later

[Brady Eidson]

> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/wtf/HashTable.h:684:9: error: static_assert failed due to requirement 'sizeof(WTF::KeyValuePair<WebCore::ProcessQualified<WTF::UUID>, WebCore::ServiceWorkerClientData>) <= 128' "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead"

Ouch.

That's an infuriating thing to creep up on this patch.

[Brady Eidson]

Created attachment 455815 [details]
EWS v2

[Brady Eidson]

Created attachment 455823 [details]
EWS v3

[Brady Eidson]

Created attachment 455849 [details]
EWS (Continuing to fight other patches that are landing)

[Brady Eidson]

Created attachment 455851 [details]
EWS yet again

[Brady Eidson]

Created attachment 455856 [details]
EWS vX

[youenn fablet]

Comment on attachment 455856 [details]
EWS vX

Patch is not r? so I did not r+ed it.
Some feedback below.


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

> Source/WTF/wtf/HashTable.h:684
> +        static_assert(sizeof(Value) <= 150, "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead");

Another approach to not get over this size would be to transmit the pageIdentifier outside of the ServiceWorkerClientData from Document to SWServer.
This makes sense especially if we we move the pageIdentifier -> ServiceWorkerClientData resolution for openWindow in NetworkProcess.
That could be done as a follow-up.

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:66
> +    return { identifier, type, frameType, url.isolatedCopy(), pageIdentifier, lastNavigationWasAppInitiated, isVisible, isFocused, focusOrder };

WTFMove(url).isolatedCopy() is a nice optimization that should allow to use URL::isolatedCopy() && (string one ref optim), let's keep it.

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:84
> +        documentPageIdentifier,

document.pageID()?

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:128
> +    }

Can you add the user activation check, hopefully it should not mess up with layout tests.
Something like:

    auto& serviceWorkerGlobalScope = downcast<ServiceWorkerGlobalScope>(context);
    if (context.settingsValues().serviceWorkersUserGestureEnabled && !serviceWorkerContext.isProcessingUserGesture()) {
        promise->reject(Exception { InvalidAccessError, "Clients openWindow requires a user gesture"_s });
        return;
    }

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:130
> +    callOnMainThread([promise = WTFMove(promise), serviceWorkerIdentifier, url = url.isolatedCopy()] () mutable {

By moving the promise ref to main thread, there is a risk it will be destroyed in the main thread (in case service worker is terminated before the call to postTaskToServiceWorker).
We can use addPendingPromise/takePendingPromise and pass an identifier instead.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:137
> +                    promise->resolveWithJSValue(JSC::jsNull());

Shouldn't we reject instead? Something like TypeError until we try passing something like an ExceptionOr<PageIdentifier>.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:141
> +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promise = WTFMove(promise)] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {

It is a bit strange to call match here.
I would be tempted to have openWindow directly return a std::optional<ServiceWorkerClientData>/ExceptionOr< ServiceWorkerClientData > instead.
The resolution can be done in NetworkProcess, provided we can have proper handling of the race conditions.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> +                        promise->resolveWithJSValue(JSC::jsNull());

Shouldn't we reject there as well?

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:174
> +    auto innerCallback = [callback = WTFMove(callback)](std::optional<WebCore::PageIdentifier>&& pageIdentifier) mutable {

It seems callback could be directly given here since innerCallback is not doing anything special.
That said, I would be tempted to do a refactoring to get the ServiceWorkerClientData from the pageIdentifier here, instead of doing that resolution in service worker process.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:178
> +    m_connection.networkProcess().parentProcessConnection()->sendWithAsyncReply(Messages::NetworkProcessProxy::OpenWindowFromServiceWorker { identifier, server->sessionID(), urlString, worker->origin().clientOrigin }, WTFMove(innerCallback));

I would be tempted to remove the service worker identifier, I am not sure identifier is useful information for UIProcess.
If we had to pass a service worker identifier to UIProcess, the service worker registration URL might be better suited?

> Source/WebKit/UIProcess/WebPageProxy.h:2085
> +    void setServiceWorkerOpenWindowCompletionCallback(CompletionHandler<void(bool)>&& completionCallback) { m_serviceWorkerOpenWindowCompletionCallback = WTFMove(completionCallback); }

We could add an ASSERT(!m_serviceWorkerOpenWindowCompletionCallback).

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2114
> +        newPage->setServiceWorkerOpenWindowCompletionCallback(WTFMove(innerCallback));

Looking quickly at the spec, I am not exactly clear when we are supposed to resolve the promise. This approach sounds good to me in any case.
Have you looked at what other browsers are doing though?

[Brady Eidson]

Created attachment 455873 [details]
One more EWS maybe?

[Brady Eidson]

(I didn't mean to ignore your feedback in my most recent patch - I simply didn't know there WAS feedback)

Addressing some now!

[Brady Eidson]

If not replying to a particular comment, I addressed it.
Other feedback inline.

(In reply to youenn fablet from comment #9)
> Comment on attachment 455856 [details]
> EWS vX
> 
> Patch is not r? so I did not r+ed it.
> Some feedback below.
> 
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455856&action=review
> 
> > Source/WTF/wtf/HashTable.h:684
> > +        static_assert(sizeof(Value) <= 150, "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead");
> 
> Another approach to not get over this size would be to transmit the
> pageIdentifier outside of the ServiceWorkerClientData from Document to
> SWServer.
> This makes sense especially if we we move the pageIdentifier ->
> ServiceWorkerClientData resolution for openWindow in NetworkProcess.
> That could be done as a follow-up.

Alex (who lowered the size to 128) wholeheartedly suggested temporarily raising it.

Especially not since this is hot code or anything.

It really belongs in the client data, and we really should uniqueRef<> all the stuff. That's the right followup patch, I think.

> 
> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:137
> > +                    promise->resolveWithJSValue(JSC::jsNull());
> 
> Shouldn't we reject instead? Something like TypeError until we try passing
> something like an ExceptionOr<PageIdentifier>.

The spec is very precise about this - If there is no valid client after the steps are run, the promise is resolved *successfully* with a value of null.

One case that happens is a blocked popup (e.g. the client refused to create a web view)

The other case is if the window that was created for the URL has navigated away to a domain that is not controlled by the SW.

So it follows....

> 
> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:141
> > +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promise = WTFMove(promise)] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {
> 
> It is a bit strange to call match here.
> I would be tempted to have openWindow directly return a
> std::optional<ServiceWorkerClientData>/ExceptionOr< ServiceWorkerClientData
> > instead.
> The resolution can be done in NetworkProcess, provided we can have proper
> handling of the race conditions.

The match'ing algorithm is used because it's very possible the page created is not controlled by the SW.

> 
> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> > +                        promise->resolveWithJSValue(JSC::jsNull());
> 
> Shouldn't we reject there as well?

See above.

> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:174
> > +    auto innerCallback = [callback = WTFMove(callback)](std::optional<WebCore::PageIdentifier>&& pageIdentifier) mutable {
> 
> It seems callback could be directly given here since innerCallback is not
> doing anything special.
> That said, I would be tempted to do a refactoring to get the
> ServiceWorkerClientData from the pageIdentifier here, instead of doing that
> resolution in service worker process.

See above for why we run the matching algorithm.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2114
> > +        newPage->setServiceWorkerOpenWindowCompletionCallback(WTFMove(innerCallback));
> 
> Looking quickly at the spec, I am not exactly clear when we are supposed to
> resolve the promise. This approach sounds good to me in any case.
> Have you looked at what other browsers are doing though?

It's resolved when the initial navigation of the new browsing context is complete (either in success or failure)

[Brady Eidson]

The talk about the matching algorithm made me realize I forgot to write a test for the non-controlled case.

Obviously need to do that.

[Brady Eidson]

Created attachment 455883 [details]
PFR v1

[Brady Eidson]

Created attachment 455888 [details]
PFR v2

[youenn fablet]

Comment on attachment 455888 [details]
PFR v2

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

> LayoutTests/http/tests/workers/service/resources/shownotification-openwindow-worker.js:16
> +    await messageClients("gotUserGestureFail");

I was initially expecting this test to fail since ServiceWorkerInternals is making sure to disable user gesture
But ServiceWorkerInternals is currently injected after the initial run of the worker script I believe.
Which makes the test pass!
When we start injecting ServiceWorkerInternals sooner, we might have to update the test to add a call to enforce the user gesture for this particular test.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> +                        promise->resolveWithJSValue(JSC::jsNull());

If there is no page identifier, it probably means the navigation failed and the promise should reject, as per https://w3c.github.io/ServiceWorker/#clients-openwindow step 7.2.
Can we add a test where the navigation fails?

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:147
> +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promiseIdentifier] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {

The work to identify the corresponding service worker can be done in network process, that would probably more efficient.
openWindow could take a completion handler taking something like Expected/ExceptionOr<std::optional<ServiceWorkerClientData>> to handle failure/null/client cases.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:175
> +        callback(WTFMove(pageIdentifier));

We could introduce a dedicated std::optional<ServiceWorkerClientData> SWServer::serviceWorkerClientFromPageIdentifier(PageIdentifier) and use it here to do the translation.
If there is no PageId, send back an exception.
If service worker is third party, return std::nullopt.
That would be functionally equivalent to matchWindowWithPageIdentifier, but a bit more efficient and ServiceWorkerClients code would look closer to the spec.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:60
> +    explicit WebsiteDataStoreClient(WKWebsiteDataStore *dataStore, id<_WKWebsiteDataStoreDelegate> delegate)

s/explicit//

[youenn fablet]

> > Source/WTF/wtf/HashTable.h:684
> > +        static_assert(sizeof(Value) <= 150, "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead");

I'll move to UniqueRef in https://bugs.webkit.org/show_bug.cgi?id=238441.

[Brady Eidson]

(In reply to youenn fablet from comment #16)
> Comment on attachment 455888 [details]
> PFR v2
> 
> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> > +                        promise->resolveWithJSValue(JSC::jsNull());
> 
> If there is no page identifier, it probably means the navigation failed and
> the promise should reject, as per
> https://w3c.github.io/ServiceWorker/#clients-openwindow step 7.2.
> Can we add a test where the navigation fails?


open window step 7.2 does *not* say throw an exception if the navigation fails.

It says throw an exception if "the algorithm steps invoked in the step labeled HandleNavigate throws an exception"

HandleNavigate invokes "Navigate" - https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate

Quickly glancing through that, "Navigate" only throws an exception "If the source browsing context is not allowed to navigate browsingContext"

In this case, the browsing context is new and is always allowed to be navigated.

A *failed* navigation (e.g. server unreachable, network error, etc) falls into:
"If newContext’s Window object’s environment settings object's creation URL's origin is not the same as the service worker's origin, resolve promise with null and abort these steps."

So it does not throw an exception, but rather returns a null WindowClient to the serviceworker.

> 
> > Source/WebCore/workers/service/ServiceWorkerClients.cpp:147
> > +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promiseIdentifier] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {
> 
> The work to identify the corresponding service worker can be done in network
> process, that would probably more efficient.
> openWindow could take a completion handler taking something like
> Expected/ExceptionOr<std::optional<ServiceWorkerClientData>> to handle
> failure/null/client cases.
> 
> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:175
> > +        callback(WTFMove(pageIdentifier));
> 
> We could introduce a dedicated std::optional<ServiceWorkerClientData>
> SWServer::serviceWorkerClientFromPageIdentifier(PageIdentifier) and use it
> here to do the translation.
> If there is no PageId, send back an exception.
> If service worker is third party, return std::nullopt.
> That would be functionally equivalent to matchWindowWithPageIdentifier, but
> a bit more efficient and ServiceWorkerClients code would look closer to the
> spec.

All of this is meant to handle a case that I don't think is necessary to handle, because we're not concerned with the known exception case of the navigation steps.

I will run a test in Firefox and Chrome to see if they agree with my reading of the specs.

[youenn fablet]

(In reply to Brady Eidson from comment #18)
> (In reply to youenn fablet from comment #16)
> > Comment on attachment 455888 [details]
> > PFR v2
> > 
> > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> > > +                        promise->resolveWithJSValue(JSC::jsNull());
> > 
> > If there is no page identifier, it probably means the navigation failed and
> > the promise should reject, as per
> > https://w3c.github.io/ServiceWorker/#clients-openwindow step 7.2.
> > Can we add a test where the navigation fails?
> 
> 
> open window step 7.2 does *not* say throw an exception if the navigation
> fails.
> 
> It says throw an exception if "the algorithm steps invoked in the step
> labeled HandleNavigate throws an exception"
> 
> HandleNavigate invokes "Navigate" -
> https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate
> 
> Quickly glancing through that, "Navigate" only throws an exception "If the
> source browsing context is not allowed to navigate browsingContext"

The network process callbacks receives an optional<PageIdentifier>.
I would believe a null PageIdentifier would mean there is something wrong going on and it would make sense to me to reject the promise.
Otherwise, if we opened the tab, we should have a PageIdentifier.
Agreed we should either resolve as null or as Client depending on the Client origin.

> In this case, the browsing context is new and is always allowed to be
> navigated.
> 
> A *failed* navigation (e.g. server unreachable, network error, etc) falls
> into:
> "If newContext’s Window object’s environment settings object's creation
> URL's origin is not the same as the service worker's origin, resolve promise
> with null and abort these steps."
> 
> So it does not throw an exception, but rather returns a null WindowClient to
> the serviceworker.

Sounds good, we got a PageIdentifier, but we will not get any client from it.

> > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:147
> > > +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promiseIdentifier] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {
> > 
> > The work to identify the corresponding service worker can be done in network
> > process, that would probably more efficient.
> > openWindow could take a completion handler taking something like
> > Expected/ExceptionOr<std::optional<ServiceWorkerClientData>> to handle
> > failure/null/client cases.
> > 
> > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:175
> > > +        callback(WTFMove(pageIdentifier));
> > 
> > We could introduce a dedicated std::optional<ServiceWorkerClientData>
> > SWServer::serviceWorkerClientFromPageIdentifier(PageIdentifier) and use it
> > here to do the translation.
> > If there is no PageId, send back an exception.
> > If service worker is third party, return std::nullopt.
> > That would be functionally equivalent to matchWindowWithPageIdentifier, but
> > a bit more efficient and ServiceWorkerClients code would look closer to the
> > spec.
> 
> All of this is meant to handle a case that I don't think is necessary to
> handle, because we're not concerned with the known exception case of the
> navigation steps.

Not really, this is a different thing, it is more about removing the need to do several IPCs and sending all clients data to service worker process while only one can be sent at the time we have the openWindow response callback from UIProcess.

[Brady Eidson]

Comment on attachment 455888 [details]
PFR v2

The iOS-wk2 failures are a preexisting mess on the bots.

[Brady Eidson]

(In reply to youenn fablet from comment #19)
> (In reply to Brady Eidson from comment #18)
> > (In reply to youenn fablet from comment #16)
> > > Comment on attachment 455888 [details]
> > > PFR v2
> > > 
> > > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> > > > +                        promise->resolveWithJSValue(JSC::jsNull());
> > > 
> > > If there is no page identifier, it probably means the navigation failed and
> > > the promise should reject, as per
> > > https://w3c.github.io/ServiceWorker/#clients-openwindow step 7.2.
> > > Can we add a test where the navigation fails?
> > 
> > 
> > open window step 7.2 does *not* say throw an exception if the navigation
> > fails.
> > 
> > It says throw an exception if "the algorithm steps invoked in the step
> > labeled HandleNavigate throws an exception"
> > 
> > HandleNavigate invokes "Navigate" -
> > https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate
> > 
> > Quickly glancing through that, "Navigate" only throws an exception "If the
> > source browsing context is not allowed to navigate browsingContext"
> 
> The network process callbacks receives an optional<PageIdentifier>.
> I would believe a null PageIdentifier would mean there is something wrong
> going on and it would make sense to me to reject the promise.
> Otherwise, if we opened the tab, we should have a PageIdentifier.
> Agreed we should either resolve as null or as Client depending on the Client
> origin.

I still disagree. 

My design is "if the client opened a window, there's a page identifier. If the client refused to open a window, there is no page identifier"

There is currently no identified case where "the client opened a window, but something went wrong that should cause us to reject"

Your design would be necessary over mine *if there was* such a case, but there isn't.
 
> > > > Source/WebCore/workers/service/ServiceWorkerClients.cpp:147
> > > > +                matchWindowWithPageIdentifier(serviceWorkerIdentifier, *pageIdentifier, [promiseIdentifier] (auto& scope, std::optional<ServiceWorkerClientData> clientData) mutable {
> > > 
> > > The work to identify the corresponding service worker can be done in network
> > > process, that would probably more efficient.
> > > openWindow could take a completion handler taking something like
> > > Expected/ExceptionOr<std::optional<ServiceWorkerClientData>> to handle
> > > failure/null/client cases.
> > > 
> > > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:175
> > > > +        callback(WTFMove(pageIdentifier));
> > > 
> > > We could introduce a dedicated std::optional<ServiceWorkerClientData>
> > > SWServer::serviceWorkerClientFromPageIdentifier(PageIdentifier) and use it
> > > here to do the translation.
> > > If there is no PageId, send back an exception.
> > > If service worker is third party, return std::nullopt.
> > > That would be functionally equivalent to matchWindowWithPageIdentifier, but
> > > a bit more efficient and ServiceWorkerClients code would look closer to the
> > > spec.
> > 
> > All of this is meant to handle a case that I don't think is necessary to
> > handle, because we're not concerned with the known exception case of the
> > navigation steps.
> 
> Not really, this is a different thing, it is more about removing the need to
> do several IPCs and sending all clients data to service worker process while
> only one can be sent at the time we have the openWindow response callback
> from UIProcess.

There's a lot of process and thread hops here in the case of responding to the SW message *and* the cases where the window would no longer be a valid client by the time the service worker process get's the reply.

I don't think pre-matching is obviously better then matching later.

But I also don't think we're going to reason our way out of it by trading bugzilla comments here.

I'm going to land the patch as-is (which I think we both agree is not *wrong*), and if we come to agreement that prematching is obviously better I think it would be a great followup patch.

[EWS]

Committed r291979 (248938@main): <https://commits.webkit.org/248938@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455888 [details].

[Fujii Hironori]

Committed r291994 (248950@trunk): <https://commits.webkit.org/248950@trunk>

[youenn fablet]

> I still disagree. 
> 
> My design is "if the client opened a window, there's a page identifier. If
> the client refused to open a window, there is no page identifier"
> 
> There is currently no identified case where "the client opened a window, but
> something went wrong that should cause us to reject"
> 
> Your design would be necessary over mine *if there was* such a case, but
> there isn't.

One such example is if the delegate is not implemented.
There will be no tab opened and it makes more sense for openWindow to reject.

> There's a lot of process and thread hops here in the case of responding to
> the SW message *and* the cases where the window would no longer be a valid
> client by the time the service worker process get's the reply.
> 
> I don't think pre-matching is obviously better then matching later.

I would not expect race conditions here, the UIProcess callback should handle these.
It is not really about matching sooner or later but a simple optimization.
Currently we do:
- UIProcess tells network process that openWindow is done, NetworkProcess notifies ServiceWorker process about this. Service Worker process gets back to network process to get all clients and finally resolves the promise
What we could do:
- UIProcess tells network process that openWindow is done, NetworkProcess translates pageID to client and sends it to ServiceWorker process. ServiceWorker process resolves promise based on this.
We basically remove one service worker process <-> Network process exchange.

I'll look at this in a follow-up.