Bug 223200 – Service worker soft-update loads not being marked app-bound

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should mark service worker soft-update loads as app-bound.

[Radar WebKit Bug Importer]

<rdar://problem/75438555>

[Kate Cheney]

Created attachment 425058 [details]
Patch

[youenn fablet]

Comment on attachment 425058 [details]
Patch

Looks good overall, a few suggestions below.

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

> Source/WebCore/workers/service/ServiceWorkerJobData.h:56
> +    bool lastNavigationWasAppBound;

ServiceWorkerJobData is serialised between Web and Networking process but lastNavigationWasAppBound use is internal to networking process.
Can we get this information from the registration directly?

> Source/WebCore/workers/service/server/SWServer.cpp:469
> +        request.setIsAppBound(jobData.lastNavigationWasAppBound);

We can probably get that information from the registration directly.
There are some timing differences in case of multiple appbound/not app bound requests to the same registration but I am not sure we can really fix it in all cases given we agglomerate soft update requests.
We can probably pass the registration to startScriptFetch instead.

> Source/WebCore/workers/service/server/SWServerRegistration.h:50
> +enum class LastNavigationWasAppBound : bool { No, Yes };

I am not sure the name is right.
The last navigation may not be appbound but the last request by a page may be app bound.
I would simplify it to IsAppBound.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:258
> +        registration->scheduleSoftUpdate(m_loader.lastNavigationWasAppBound() ? WebCore::LastNavigationWasAppBound::Yes : WebCore::LastNavigationWasAppBound::No);

I would rename it to isAppBound() since this is what it really is.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1777
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "successfully registered");

Shouldn't we reset the appBoundNavigationData here?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1782
> +    [webView loadRequest:request];

We need to make sure this request alone will not hit the network so that only soft update will actually change the appbound navigation data.

[Kate Cheney]

Comment on attachment 425058 [details]
Patch

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

>> Source/WebCore/workers/service/ServiceWorkerJobData.h:56
>> +    bool lastNavigationWasAppBound;
> 
> ServiceWorkerJobData is serialised between Web and Networking process but lastNavigationWasAppBound use is internal to networking process.
> Can we get this information from the registration directly?

Yes, will fix.

>> Source/WebCore/workers/service/server/SWServer.cpp:469
>> +        request.setIsAppBound(jobData.lastNavigationWasAppBound);
> 
> We can probably get that information from the registration directly.
> There are some timing differences in case of multiple appbound/not app bound requests to the same registration but I am not sure we can really fix it in all cases given we agglomerate soft update requests.
> We can probably pass the registration to startScriptFetch instead.

Good point, will fix this in the next patch.

>> Source/WebCore/workers/service/server/SWServerRegistration.h:50
>> +enum class LastNavigationWasAppBound : bool { No, Yes };
> 
> I am not sure the name is right.
> The last navigation may not be appbound but the last request by a page may be app bound.
> I would simplify it to IsAppBound.

That makes sense. Will change.

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:258
>> +        registration->scheduleSoftUpdate(m_loader.lastNavigationWasAppBound() ? WebCore::LastNavigationWasAppBound::Yes : WebCore::LastNavigationWasAppBound::No);
> 
> I would rename it to isAppBound() since this is what it really is.

Ditto, will change.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1777
>> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "successfully registered");
> 
> Shouldn't we reset the appBoundNavigationData here?

Yes we can, that will make the test more robust I think.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1782
>> +    [webView loadRequest:request];
> 
> We need to make sure this request alone will not hit the network so that only soft update will actually change the appbound navigation data.

I guess that is true. Although I wonder if it is enough to know that the soft-update got loaded (which I think is indicated from the "worker already active" alert) and then check the app-bound navigation data. If the load was incorrectly marked, the opposite boolean value will have the wrong value.

[Kate Cheney]

Created attachment 425303 [details]
Patch

[Kate Cheney]

Comment on attachment 425058 [details]
Patch

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

New patch posted. Thanks for the comments.

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1782
>>> +    [webView loadRequest:request];
>> 
>> We need to make sure this request alone will not hit the network so that only soft update will actually change the appbound navigation data.
> 
> I guess that is true. Although I wonder if it is enough to know that the soft-update got loaded (which I think is indicated from the "worker already active" alert) and then check the app-bound navigation data. If the load was incorrectly marked, the opposite boolean value will have the wrong value.

I added didScheduleSoftUpdate to the testing data. This will track if a soft update occurred for these tests so we know the soft update is changing the app-bound data we get from the network process.

[Kate Cheney]

Created attachment 425310 [details]
Patch

[youenn fablet]

Comment on attachment 425310 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServerRegistration.h:111
> +    void setIsAppBound(bool isAppBound) { m_isAppBound = isAppBound; }

This setter does not seem needed given we set m_isAppBound in scheduleSoftUpdate.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1764
> +        { "application/javascript", "" },

As I see it, the service worker script is empty so the second web page load will go to the network process.
I think it would be better if we were using the following js:
"self.addEventListener('fetch', (event) => { event.respondWith(new Response(new Blob(['<script>alert(\"synthetic response\")</script>'], {type: 'text/html'}))); })"

For the second load page, we would have something like:
EXPECT_WK_STREQ([webView _test_waitForAlert], "synthetic response");

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

This might be racy once only the soft update load happens given there is a timer of 1 second between the scheduling of the soft update and the actual soft update.

[Kate Cheney]

Comment on attachment 425310 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServerRegistration.h:111
>> +    void setIsAppBound(bool isAppBound) { m_isAppBound = isAppBound; }
> 
> This setter does not seem needed given we set m_isAppBound in scheduleSoftUpdate.

Good point, will remove.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1764
>> +        { "application/javascript", "" },
> 
> As I see it, the service worker script is empty so the second web page load will go to the network process.
> I think it would be better if we were using the following js:
> "self.addEventListener('fetch', (event) => { event.respondWith(new Response(new Blob(['<script>alert(\"synthetic response\")</script>'], {type: 'text/html'}))); })"
> 
> For the second load page, we would have something like:
> EXPECT_WK_STREQ([webView _test_waitForAlert], "synthetic response");

If I change the js to return a synthetic response, I think the test exits earlier than the soft update happens. I maybe need some way to wait for the soft update before exiting.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1794
>> +    [webView _appBoundNavigationData: ^(struct WKAppBoundNavigationTestingData data) {
> 
> This might be racy once only the soft update load happens given there is a timer of 1 second between the scheduling of the soft update and the actual soft update.

Ok, I might use this to make sure we wait for the soft-update to finish as mentioned above.

[Kate Cheney]

Comment on attachment 425310 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1764
>>> +        { "application/javascript", "" },
>> 
>> As I see it, the service worker script is empty so the second web page load will go to the network process.
>> I think it would be better if we were using the following js:
>> "self.addEventListener('fetch', (event) => { event.respondWith(new Response(new Blob(['<script>alert(\"synthetic response\")</script>'], {type: 'text/html'}))); })"
>> 
>> For the second load page, we would have something like:
>> EXPECT_WK_STREQ([webView _test_waitForAlert], "synthetic response");
> 
> If I change the js to return a synthetic response, I think the test exits earlier than the soft update happens. I maybe need some way to wait for the soft update before exiting.

Also I think even with the synthetic response there will still be one load to the network process from the main request, or at least that is what I see in the test. Without the synthetic response there were 2.

[Kate Cheney]

Created attachment 425917 [details]
Patch

[Kate Cheney]

Ok, latest patch addresses the changes you mentioned. First, it uses the synthetic response JS code you suggested instead of an empty string. I also updated the test to continuously check for a soft update until one has occurred to account for the delay between the update and the synthetic response, and I moved the code that changes the AppBoundNavigationTestingData.didPerformSoftUpdate value to be when the actual soft update load occurs, not when it is scheduled. This should fix any raciness, I confirmed by running 200 iterations.

I also was having local timeout issues with ServiceWorkerTCPServer (I filed rdar://76611319 to investigate) so I switched to TestWebKitAPI::HTTPServer, but the soft update logic is the same.

Waiting on EWS before landing.

Thanks for the help Youenn!

[Kate Cheney]

Created attachment 425927 [details]
Patch

[EWS]

Committed r275952 (236512@main): <https://commits.webkit.org/236512@main>

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