Bug 223200 - Service worker soft-update loads not being marked app-bound
Summary: Service worker soft-update loads not being marked app-bound
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-15 11:31 PDT by Kate Cheney
Modified: 2021-04-14 10:54 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.91 KB, patch)
2021-04-02 15:01 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2021-04-06 11:14 PDT, Kate Cheney
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.75 KB, patch)
2021-04-06 12:33 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (27.06 KB, patch)
2021-04-13 14:47 PDT, Kate Cheney
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (27.09 KB, patch)
2021-04-13 16:01 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-03-15 11:31:03 PDT
We should mark service worker soft-update loads as app-bound.
Comment 1 Radar WebKit Bug Importer 2021-03-15 11:31:20 PDT
<rdar://problem/75438555>
Comment 2 Kate Cheney 2021-04-02 15:01:07 PDT
Created attachment 425058 [details]
Patch
Comment 3 youenn fablet 2021-04-05 10:45:21 PDT
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.
Comment 4 Kate Cheney 2021-04-06 10:32:17 PDT
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.
Comment 5 Kate Cheney 2021-04-06 11:14:00 PDT
Created attachment 425303 [details]
Patch
Comment 6 Kate Cheney 2021-04-06 11:14:47 PDT
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.
Comment 7 Kate Cheney 2021-04-06 12:33:56 PDT
Created attachment 425310 [details]
Patch
Comment 8 youenn fablet 2021-04-07 04:43:37 PDT
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.
Comment 9 Kate Cheney 2021-04-13 13:04:00 PDT
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.
Comment 10 Kate Cheney 2021-04-13 13:18:16 PDT
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.
Comment 11 Kate Cheney 2021-04-13 14:47:08 PDT
Created attachment 425917 [details]
Patch
Comment 12 Kate Cheney 2021-04-13 14:48:25 PDT
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!
Comment 13 Kate Cheney 2021-04-13 16:01:26 PDT
Created attachment 425927 [details]
Patch
Comment 14 EWS 2021-04-14 10:54:12 PDT
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].