Bug 222635 – Service worker loads are not marked as app-bound

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

There is a bug where loads via service worker are not being marked as app-bound requests.

[Kate Cheney]

rdar://74395950

[Kate Cheney]

Created attachment 422025 [details]
Patch

[youenn fablet]

Comment on attachment 422025 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:448
> +    server.setLastNavigationWasAppBound(loadParameters.request.isAppBound());

server is responsible for all pages, so it might be that it will start a service worker but not related to this load.
I think you should try to pass this parameter value to runServiceWorkerIfNecessary.
This should ultimately go to installServiceWorkerContext call.

As a first step you might want to do that in WebSWServerConnection::startFetch, where this should be related to a NetworkResourceLoader, so you should be able to get isAppBound().
As a second step, you might also want to do the same thing for events, currently it is in WebSWServerConnection::postMessageToServiceWorker, which can probably try to get it from WebProcess through IPC.
This could be done in a follow-up though, as you will need to pass this info to WebSWClientConnection::postMessageToServiceWorker.

[Kate Cheney]

Created attachment 422726 [details]
Patch

[Kate Cheney]

Comment on attachment 422025 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:448
>> +    server.setLastNavigationWasAppBound(loadParameters.request.isAppBound());
> 
> server is responsible for all pages, so it might be that it will start a service worker but not related to this load.
> I think you should try to pass this parameter value to runServiceWorkerIfNecessary.
> This should ultimately go to installServiceWorkerContext call.
> 
> As a first step you might want to do that in WebSWServerConnection::startFetch, where this should be related to a NetworkResourceLoader, so you should be able to get isAppBound().
> As a second step, you might also want to do the same thing for events, currently it is in WebSWServerConnection::postMessageToServiceWorker, which can probably try to get it from WebProcess through IPC.
> This could be done in a follow-up though, as you will need to pass this info to WebSWClientConnection::postMessageToServiceWorker.

I posted a new patch marking the SW document loader in WebSWContextManagerConnection::startFetch() based on the request value. I am still not sure of the best way to get the correct default value in the case of a fetch called in a postMessage event handler.

[youenn fablet]

Comment on attachment 422726 [details]
Patch

Can you add a test where service worker is serving two pages, one being app bound and the other being not app bound.
Also maybe do some loads outside of the fetch event handler (async fetch for instance).

[Chris Dumez]

Comment on attachment 422726 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:129
> +    m_document->loader()->setLastNavigationWasAppBound(wasAppBound);

Shouldn't we null check the DocumentLoader?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1562
> +static void runTest(ResponseType responseType, bool appBound)

bool appBound would look better at call site if we used an enum class.

enum class IsAppBound : bool { No, Yes };

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1567
> +    __block bool removedAnyExistingData = false;

Seems like you could use the existing isDone above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1625
> +        NSMutableURLRequest *nonAppBoundRequest = [request mutableCopy];

This seems to be leaking. If you call mutableCopy, you should adoptNS() right away.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1639
> +    bool expectingAppBoundRequests = appBound ? true : false;

Why aren't you using appBound directly? What's the point of this new boolean that means exactly the same thing?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1645
> +    }];

Aren't you missing this right after this call?

TestWebKitAPI::Util::run(&isDone);

[youenn fablet]

Comment on attachment 422726 [details]
Patch

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

>> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:129
>> +    m_document->loader()->setLastNavigationWasAppBound(wasAppBound);
> 
> Shouldn't we null check the DocumentLoader?

It might not hurt but in practice ServiceWorkerThreadProxy document will not become frameless.
The page, main frame and document relationship should stay the same throughout the lifetime of the service worker.

[Kate Cheney]

Created attachment 422791 [details]
Patch

[Kate Cheney]

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

Covers more cases, like a fetch event from a post message and a soft update. 

I am not sure I wrote the async fetch test correctly, though.

[youenn fablet]

EWS failure seems related.
I think it would be best to split the patch in several pieces.
Start with the easy one that you did in https://bugs.webkit.org/attachment.cgi?id=422726&action=review with some improved tests.
Then follow-up patches for soft update, computing the default value and postMessage.

As of service worker async test, I missed that we report the app bound request states through a WKWebView.
Service workers might do fetches for several web views so even though we have a heuristic that tells us whether a request done by a service worker is app bound or not, we probably want to define a heuristic to attach these loads to a particular WKWebView (or we would skip these loads or define an API that is not WKWebView specific).
The heuristic could be last one wins, meaning that we tie service worker loads to the last service worker window client that triggered either a fetch or a message event.
In case this window client disappears, we would need to pick another window client.

[Kate Cheney]

Created attachment 423238 [details]
Patch

[Kate Cheney]

(In reply to youenn fablet from comment #11)
> EWS failure seems related.
> I think it would be best to split the patch in several pieces.
> Start with the easy one that you did in
> https://bugs.webkit.org/attachment.cgi?id=422726&action=review with some
> improved tests.
> Then follow-up patches for soft update, computing the default value and
> postMessage.
> 

Ok, new simple-case patch uploaded. Tracking the other fixes here: https://bugs.webkit.org/show_bug.cgi?id=223201 and https://bugs.webkit.org/show_bug.cgi?id=223200. I think the default case will fall into one of these two.


> As of service worker async test, I missed that we report the app bound
> request states through a WKWebView.
> Service workers might do fetches for several web views so even though we
> have a heuristic that tells us whether a request done by a service worker is
> app bound or not, we probably want to define a heuristic to attach these
> loads to a particular WKWebView (or we would skip these loads or define an
> API that is not WKWebView specific).
> The heuristic could be last one wins, meaning that we tie service worker
> loads to the last service worker window client that triggered either a fetch
> or a message event.
> In case this window client disappears, we would need to pick another window
> client.

I excluded the async test case from the latest patch, but I don't fully understand why we need to attach loads to a particular WebView. If we mark a fetch based on the initiating request, won't that always be associated with the correct WebView?

[youenn fablet]

Comment on attachment 423238 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1639
> +    [webView _appBoundNavigationData: ^(struct WKAppBoundNavigationTestingData data) {

Could we do that check before reloading as well.
Is there a way to clear the appBoundNavigationData as well?
I would guess that for synthetic data, in the reload case, we would probably not see any load at all from the web page.

[Kate Cheney]

Comment on attachment 423238 [details]
Patch

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

Thanks for the review!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1639
>> +    [webView _appBoundNavigationData: ^(struct WKAppBoundNavigationTestingData data) {
> 
> Could we do that check before reloading as well.
> Is there a way to clear the appBoundNavigationData as well?
> I would guess that for synthetic data, in the reload case, we would probably not see any load at all from the web page.

Yes, I will fix this in the patch-for-landing.

[Kate Cheney]

Created attachment 424081 [details]
Patch for landing

[EWS]

Committed r274928: <https://commits.webkit.org/r274928>

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