We should mark service worker soft-update loads as app-bound.
<rdar://problem/75438555>
Created attachment 425058 [details] Patch
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 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.
Created attachment 425303 [details] Patch
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.
Created attachment 425310 [details] Patch
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 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 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.
Created attachment 425917 [details] Patch
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!
Created attachment 425927 [details] Patch
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].