Bug 223201 – Mark ServiceWorkerThreadProxy with a default app-bound value

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should mark service worker postMessage events as app-bound.

[Radar WebKit Bug Importer]

<rdar://problem/75438678>

[Kate Cheney]

<rdar://problem/77664416>

[Kate Cheney]

Created attachment 428574 [details]
Patch

[youenn fablet]

Comment on attachment 428574 [details]
Patch

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:582
> +        auto contextData = ServiceWorkerContextData { WTF::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(*certificateInfo), WTFMove(*contentSecurityPolicy), WTFMove(referrerPolicy), WTFMove(scriptURL), *workerType, true, false, WTFMove(scriptResourceMap) };

true and false here are a bit mysterious.

> Source/WebCore/workers/service/server/SWServer.cpp:651
> +    tryInstallContextData(ServiceWorkerContextData { jobDataIdentifier, registration.data(), ServiceWorkerIdentifier::generate(), script, certificateInfo, contentSecurityPolicy, referrerPolicy, url, type, false, false, WTFMove(scriptResourceMap) });

false and false are a bit mysterious.
Maybe we should have a bool enum here for the new field.

> Source/WebCore/workers/service/server/SWServer.cpp:668
> +    }

Maybe we could have a single rule here: a service worker appbound state is true if any of its client is appbound, and stick with it.
Then we would not need to change appbound state based on postMessage or network load.
Anytime a new client is added/removed, we update the appbound state of all related SWServerWorker.
If a worker is running, we send an IPC to change its appbound state. Otherwise we wait for the worker to run and send the appbound state as part of the context data.


> Source/WebCore/workers/service/server/SWServer.h:234
> +    bool runServiceWorker(ServiceWorkerIdentifier, LastNavigationWasAppBound);

I would tend to pass an Optional<LastNavigationWasAppBound>.

> Source/WebCore/workers/service/server/SWServerWorker.cpp:89
> +    return { WTF::nullopt, m_registration->data(), m_data.identifier, m_script, m_certificateInfo, m_contentSecurityPolicy, m_referrerPolicy, m_data.scriptURL, m_data.type, false, false, m_scriptResourceMap };

I would compute the exact appbound state here using its m_server and its client origin.
This should remove the need to pass the value in various places.

> Tools/ChangeLog:12
> +        when the code paths were being hit.

If needed, you can probably add an API in ServiceWorkerInternals and I guess it can be injected through WKBundleSetServiceWorkerProxyCreationCallback.

[Kate Cheney]

Comment on attachment 428574 [details]
Patch

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

Thanks for the help!

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:582
>> +        auto contextData = ServiceWorkerContextData { WTF::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(*certificateInfo), WTFMove(*contentSecurityPolicy), WTFMove(referrerPolicy), WTFMove(scriptURL), *workerType, true, false, WTFMove(scriptResourceMap) };
> 
> true and false here are a bit mysterious.

I can make them into enum values instead.

>> Source/WebCore/workers/service/server/SWServer.cpp:651
>> +    tryInstallContextData(ServiceWorkerContextData { jobDataIdentifier, registration.data(), ServiceWorkerIdentifier::generate(), script, certificateInfo, contentSecurityPolicy, referrerPolicy, url, type, false, false, WTFMove(scriptResourceMap) });
> 
> false and false are a bit mysterious.
> Maybe we should have a bool enum here for the new field.

Will do.

>> Source/WebCore/workers/service/server/SWServer.cpp:668
>> +    }
> 
> Maybe we could have a single rule here: a service worker appbound state is true if any of its client is appbound, and stick with it.
> Then we would not need to change appbound state based on postMessage or network load.
> Anytime a new client is added/removed, we update the appbound state of all related SWServerWorker.
> If a worker is running, we send an IPC to change its appbound state. Otherwise we wait for the worker to run and send the appbound state as part of the context data.

Ok, good idea, hopefully this will be simpler. And we can link a client to a SWServerWorker by the top origin, right?

>> Source/WebCore/workers/service/server/SWServer.h:234
>> +    bool runServiceWorker(ServiceWorkerIdentifier, LastNavigationWasAppBound);
> 
> I would tend to pass an Optional<LastNavigationWasAppBound>.

Ok, will change.

>> Source/WebCore/workers/service/server/SWServerWorker.cpp:89
>> +    return { WTF::nullopt, m_registration->data(), m_data.identifier, m_script, m_certificateInfo, m_contentSecurityPolicy, m_referrerPolicy, m_data.scriptURL, m_data.type, false, false, m_scriptResourceMap };
> 
> I would compute the exact appbound state here using its m_server and its client origin.
> This should remove the need to pass the value in various places.

Don't recall why I didn't do that -- I'll fix it.

>> Tools/ChangeLog:12
>> +        when the code paths were being hit.
> 
> If needed, you can probably add an API in ServiceWorkerInternals and I guess it can be injected through WKBundleSetServiceWorkerProxyCreationCallback.

Ok, will make changes then see if I need to do this.

[Kate Cheney]

Created attachment 428885 [details]
Patch

[youenn fablet]

Comment on attachment 428885 [details]
Patch

Looks better to me.
Last comment might further simplifies things, to be checked though.

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

> Source/WebCore/ChangeLog:22
> +        marked app-bound based on the document loader.

Is there a way to test this change?

> Source/WebCore/workers/service/server/SWServerWorker.cpp:91
> +        isAppBound = m_server->clientIsAppBoundForRegistrableDomain(m_origin.value().clientRegistrableDomain());

Maybe we do not even need this check here.
We could make SWServerWorker have a LastNavigationWasAppBound field.
On creation, it is set according current clients.
On added/removed client, it gets also updated.
If the SWServerWorker is running and its LastNavigationWasAppBound changes, it will call WebSWServerToContextConnection::updateAppBoundValue through its SWServer.

[Kate Cheney]

Comment on attachment 428885 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +        marked app-bound based on the document loader.
> 
> Is there a way to test this change?

This might be better in another patch with a test I think.

>> Source/WebCore/workers/service/server/SWServerWorker.cpp:91
>> +        isAppBound = m_server->clientIsAppBoundForRegistrableDomain(m_origin.value().clientRegistrableDomain());
> 
> Maybe we do not even need this check here.
> We could make SWServerWorker have a LastNavigationWasAppBound field.
> On creation, it is set according current clients.
> On added/removed client, it gets also updated.
> If the SWServerWorker is running and its LastNavigationWasAppBound changes, it will call WebSWServerToContextConnection::updateAppBoundValue through its SWServer.

I see. Ok let me try with a new patch.

[Kate Cheney]

Created attachment 429030 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #9)
> Created attachment 429030 [details]
> Patch

Uploading this without the review flag since I am having trouble getting the ServiceWorkerInternals test API to work (all code is in the patch). Youenn I am wondering if you would be willing to take a look and see if you notice anything wrong?

Another thought: do we need to update the ServiceWorkerThreadProxy app-bound value in WebSWContextManagerConnection::startFetch now that this heuristic is in place? This was done in a previous patch. I am getting false positives in my test cases because startFetch() sets the correct app-bound value in addition to the worker being updated when registering an app-bound client.

[youenn fablet]

Comment on attachment 429030 [details]
Patch

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

> Source/WebCore/testing/ServiceWorkerInternals.cpp:117
> +            promise->reject(TypeError);

We want to hop back to the worker thread before resolving/rejecting the promise.
Something like:

ASSERT(! m_lastNavigationWasAppBoundPromise);
m_lastNavigationWasAppBoundPromise = WTFMove(promise);
callOnMainThread([identifier = m_identifier, weakThis = makeWeakPtr(this)]() mutable {
    proxy->thread().runLoop().postTaskForMode(weakThis  = WTFMove(weakThis), appBound = proxy->lastNavigationWasAppBound()](auto&) {
        if (!weakThis || !weakThis->m_lastNavigationWasAppBoundPromise)
            return;
        weakThis->m_lastNavigationWasAppBoundPromise->resolve<IDLBoolean>(appBound);
        weakThis->m_lastNavigationWasAppBoundPromise = { };
    }
});

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:72
> +        lastNavigationWasAppBound = document.loader()->lastNavigationWasAppBound() ? LastNavigationWasAppBound::Yes : LastNavigationWasAppBound::No;

Could use a oneliner:
auto lastNavigationWasAppBound = document.loader() && document.loader()->lastNavigationWasAppBound() ? LastNavigationWasAppBound::Yes : ...;

> Source/WebCore/workers/service/server/SWServer.cpp:664
> +            continue;

I would think we do not need this if check, maybe add an ASSERT instead

> Source/WebCore/workers/service/server/SWServerWorker.cpp:97
> +        return;

We probably want to set m_lastNavigationWasAppBound even if running.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:151
> +    data.lastNavigationWasAppBound = request.isAppBound() ? WebCore::LastNavigationWasAppBound::Yes : WebCore::LastNavigationWasAppBound::No;

Can we set lastNavigationWasAppBound in data constructor directly?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1887
> +    alert('App-bound value ' + value);

What we usually do is the following:
- load the web page
- web page creates a service worker
- when ready, post message from web page to service worker to trigger a test.
- service worker does the test and returns the result back to the web page using postMessage
- web page sends the result of the test to the API test using message handlers or alert for instance.
See ServiceWorkerBasic for some examples.

In your case, the call to internals.lastNavigationWasAppBound() should be done in the script run by the service worker (the variable called js below).
Something like:
auto js = """onmessage = async (event) => {
    if (!self.internals) {
        event.source.postMessage("No internals");
        return;
    }
    event.source.postMessage(await internals.lastNavigationWasAppBound());
}""";

[Kate Cheney]

Comment on attachment 429030 [details]
Patch

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

>> Source/WebCore/testing/ServiceWorkerInternals.cpp:117
>> +            promise->reject(TypeError);
> 
> We want to hop back to the worker thread before resolving/rejecting the promise.
> Something like:
> 
> ASSERT(! m_lastNavigationWasAppBoundPromise);
> m_lastNavigationWasAppBoundPromise = WTFMove(promise);
> callOnMainThread([identifier = m_identifier, weakThis = makeWeakPtr(this)]() mutable {
>     proxy->thread().runLoop().postTaskForMode(weakThis  = WTFMove(weakThis), appBound = proxy->lastNavigationWasAppBound()](auto&) {
>         if (!weakThis || !weakThis->m_lastNavigationWasAppBoundPromise)
>             return;
>         weakThis->m_lastNavigationWasAppBoundPromise->resolve<IDLBoolean>(appBound);
>         weakThis->m_lastNavigationWasAppBoundPromise = { };
>     }
> });

Will change.

>> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:72
>> +        lastNavigationWasAppBound = document.loader()->lastNavigationWasAppBound() ? LastNavigationWasAppBound::Yes : LastNavigationWasAppBound::No;
> 
> Could use a oneliner:
> auto lastNavigationWasAppBound = document.loader() && document.loader()->lastNavigationWasAppBound() ? LastNavigationWasAppBound::Yes : ...;

Will fix.

>> Source/WebCore/workers/service/server/SWServer.cpp:664
>> +            continue;
> 
> I would think we do not need this if check, maybe add an ASSERT instead

Will do.

>> Source/WebCore/workers/service/server/SWServerWorker.cpp:97
>> +        return;
> 
> We probably want to set m_lastNavigationWasAppBound even if running.

Ah, good point. Will fix.

>> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:151
>> +    data.lastNavigationWasAppBound = request.isAppBound() ? WebCore::LastNavigationWasAppBound::Yes : WebCore::LastNavigationWasAppBound::No;
> 
> Can we set lastNavigationWasAppBound in data constructor directly?

Yes. That will fix the bots also.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1887
>> +    alert('App-bound value ' + value);
> 
> What we usually do is the following:
> - load the web page
> - web page creates a service worker
> - when ready, post message from web page to service worker to trigger a test.
> - service worker does the test and returns the result back to the web page using postMessage
> - web page sends the result of the test to the API test using message handlers or alert for instance.
> See ServiceWorkerBasic for some examples.
> 
> In your case, the call to internals.lastNavigationWasAppBound() should be done in the script run by the service worker (the variable called js below).
> Something like:
> auto js = """onmessage = async (event) => {
>     if (!self.internals) {
>         event.source.postMessage("No internals");
>         return;
>     }
>     event.source.postMessage(await internals.lastNavigationWasAppBound());
> }""";

I see -- I was calling it in the wrong place. Ok let me see if I can get it to work like this. Thanks for the help and explanation.

[Kate Cheney]

Created attachment 429090 [details]
Patch

[youenn fablet]

Comment on attachment 429090 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:894
> +    }

Another approach would be to use m_clientsByRegistrableDomain to get the list of service worker IDs for the client origin.
And then use m_runningOrTerminatingWorkers to get the worker from its id.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:2009
> +    }, TestWebKitAPI::HTTPServer::Protocol::Https);

Why do we need to create two servers? Can we just use one?

[Kate Cheney]

Comment on attachment 429090 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServer.cpp:894
>> +    }
> 
> Another approach would be to use m_clientsByRegistrableDomain to get the list of service worker IDs for the client origin.
> And then use m_runningOrTerminatingWorkers to get the worker from its id.

I am not sure this will work. m_clientsByRegistrableDomain will give a list of ServiceWorkerClientIdentifier objects, but we need the ServiceWorkerIdentifier objects to get the workers. Maybe I am misunderstanding? I can post a followup if so.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:2009
>> +    }, TestWebKitAPI::HTTPServer::Protocol::Https);
> 
> Why do we need to create two servers? Can we just use one?

I was hitting timeouts using ServiceWorkerTCPServer (tracked in rdar://76611319), which I think is the only place you can use one server for multiple WebView loads with different js? Let me see if I can get it working with 1 HTTP server before landing.

[youenn fablet]

Comment on attachment 429090 [details]
Patch

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

>>> Source/WebCore/workers/service/server/SWServer.cpp:894
>>> +    }
>> 
>> Another approach would be to use m_clientsByRegistrableDomain to get the list of service worker IDs for the client origin.
>> And then use m_runningOrTerminatingWorkers to get the worker from its id.
> 
> I am not sure this will work. m_clientsByRegistrableDomain will give a list of ServiceWorkerClientIdentifier objects, but we need the ServiceWorkerIdentifier objects to get the workers. Maybe I am misunderstanding? I can post a followup if so.

No, you are right, your approach is fine.

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:2009
>>> +    }, TestWebKitAPI::HTTPServer::Protocol::Https);
>> 
>> Why do we need to create two servers? Can we just use one?
> 
> I was hitting timeouts using ServiceWorkerTCPServer (tracked in rdar://76611319), which I think is the only place you can use one server for multiple WebView loads with different js? Let me see if I can get it working with 1 HTTP server before landing.

I think TCP server will answer a given number of requests.

[Kate Cheney]

Created attachment 429235 [details]
Patch for landing

[Kate Cheney]

(In reply to youenn fablet from comment #16)
> Comment on attachment 429090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429090&action=review
> 
> >>> Source/WebCore/workers/service/server/SWServer.cpp:894
> >>> +    }
> >> 
> >> Another approach would be to use m_clientsByRegistrableDomain to get the list of service worker IDs for the client origin.
> >> And then use m_runningOrTerminatingWorkers to get the worker from its id.
> > 
> > I am not sure this will work. m_clientsByRegistrableDomain will give a list of ServiceWorkerClientIdentifier objects, but we need the ServiceWorkerIdentifier objects to get the workers. Maybe I am misunderstanding? I can post a followup if so.
> 
> No, you are right, your approach is fine.
> 
> >>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:2009
> >>> +    }, TestWebKitAPI::HTTPServer::Protocol::Https);
> >> 
> >> Why do we need to create two servers? Can we just use one?
> > 
> > I was hitting timeouts using ServiceWorkerTCPServer (tracked in rdar://76611319), which I think is the only place you can use one server for multiple WebView loads with different js? Let me see if I can get it working with 1 HTTP server before landing.
> 
> I think TCP server will answer a given number of requests.

Got it working with one TCP server.

[EWS]

Committed r277837 (237981@main): <https://commits.webkit.org/237981@main>

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

[Chris Dumez]

Comment on attachment 429235 [details]
Patch for landing

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:120
> +    if (data.lastNavigationWasAppBound)

Use-after-move of |data|