Bug 237160

<rdar://problem/89440067>

Failure was reproducible at iOS15 Production TOT - r290435.

Regression range appears to be r290356-r290351.

Marking expect

(In reply to Matteo Flores from comment #3)
> Marking expect

Marking expectations since these tests are affecting ews negatively.*

Created attachment 453143 [details]
expectations

Comment on attachment 453143 [details]
expectations

Clearing flags on attachment: 453143

Committed r290469 (247768@trunk): <https://commits.webkit.org/247768@trunk>

*** Bug 237161 has been marked as a duplicate of this bug. ***

*** Bug 237162 has been marked as a duplicate of this bug. ***

Since all three of these appear to go together, I have marked my bugs as duplicates. Though, expectations were still landed on them here:
https://trac.webkit.org/changeset/290466/webkit

and 

https://trac.webkit.org/changeset/290468/webkit

So when this is resolved, those expectations will need to be removed.

The only meaningful revision that could be related is https://trac.webkit.org/changeset/290352/webkit

Diff 1:
--- /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https-expected.txt
+++ /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https-actual.txt
@@ -1,4 +1,4 @@
 
 PASS Verify matchAll() with window client type
-FAIL Verify matchAll() with {window, sharedworker, worker} client types promise_test: Unhandled rejection with value: object "TypeError: null is not an object (evaluating 'w.port.onmessage = _ => resolve(w)')"
+FAIL Verify matchAll() with {window, sharedworker, worker} client types assert_equals: result count expected 1 but got 0
 

Diff 2:
--- /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker.tentative.https-expected.txt
+++ /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker.tentative.https-actual.txt
@@ -1,6 +1,6 @@
 The 3p iframe's postMessage:
-3p iframe's has_pending: false source: From3pFrame. 1p iframe's has_pending: true source: From1pFrame
+3p iframe's has_pending: false source: From3pFrame.
 
 
-PASS Services workers under different top-level sites are partitioned.
+FAIL Services workers under different top-level sites are partitioned. assert_equals: The 1p frames were serviced by different service workers. expected 0.2187869892862967 but got 0.9404173978803455


Diff 3:
--- /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker-getRegistrations.tentative.https-expected.txt
+++ /Volumes/Data/worker/ios-simulator-15-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker-getRegistrations.tentative.https-actual.txt
@@ -1,4 +1,4 @@
 This test loads a SW in a first-party context and gets the SW's (randomly) generated ID. It does the same thing for the SW but in a third-party context and then confirms that the IDs are different.
 
-PASS ServiceWorker's getRegistration() is partitioned
+FAIL ServiceWorker's getRegistration() is partitioned assert_true: 1p SW didn't shutdown expected true got false

The first diff doesn't really look like a regression but the other 2 look more concerning. I will investigate.

FYI, if you go to:
https://wpt.live/service-workers/service-worker/partitioned-service-worker-getRegistrations.tentative.https.html
https://wpt.live/service-workers/service-worker/partitioned-service-worker-getRegistrations.tentative.https.html

in Shipping Safari (on macOS), you'll see that they were already flaky. They fail on first load then succeed on reload (or if the service worker is already running).

I think my patch at r290352 just made this more obvious and we're now failing more consistently. Still worth investigating though. I think this means we're failing to implement part of the spec and somehow we were getting lucky and still passing in test in the context of our layout tests.

Ok, so here is what seems to be happening (I looked at partitioned-service-worker-getRegistrations.tentative.https.html but I am almost certain the issue is the same for the other one).

The test relies on an ID js variable that lives inside the service worker. The test expects this ID to not change during the execution of the test.

The test initially launches a service worker for its origin. Thanks to my change in r290352, we now run this service worker in the same WebProcess as the test (better for perf and memory usage).
The issue is that the test then loads another origin (in an iframe and in a new window). As a result, WebProcessProxy::didStartProvisionalLoadForMainFrame(), we determine that the WebContent process is no longer "origin-clean" (it has loaded more than one eTLD+1). When this happens, we have logic to terminate the service worker that's inside the current process and relaunch it is a new "origin-clean" process.

Because we relaunch the service worker, the ID variable changes and it confuses the test.

Technically, we could say that the test is wrong to assume the service worker will not exit/relaunch during the execution of the test, since service workers can famously be killed at almost any point.
However, the test is relying on FetchEvent.waitUntil() (with a promise that it doesn't resolve until the very end of the test). Per the specification, I believe we're not supposed to exit service workers that have pending events.

So while r290352 made the issue obvious, this is not really a regression from r290352. We were getting lucky before and the service worker was launching in another WebContent process (likely a dedicated one) from the beginning. As a result, the service worker would not relaunch when the test would start loading different origins.

Created attachment 453650 [details]
WIP patch

Created attachment 453662 [details]
Patch

Created attachment 453663 [details]
Patch

Comment on attachment 453663 [details]
Patch

For shared workers, I would tend to use a different process:
- this is slightly better from a security point of view.
- web sites may start to rely on the 10 seconds delay for shared worker which might become flaky behavior.
- we do not have the same launch speed requirement for shared workers as we have for service workers.

Comment on attachment 453663 [details]
Patch

Let’s discuss the shared workers separately.

Tools/Scripts/svn-apply failed to apply attachment 453663 [details] to trunk.
Please resolve the conflicts and upload a new patch.

Created attachment 453733 [details]
Patch

Committed r290776 (248020@main): <https://commits.webkit.org/248020@main>

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

After the changes in https://github.com/WebKit/WebKit/commit/2fe065c2c83cfd99bcad7214e74ac72c6a49a4e6

two of the tests are now working again. but imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-no-freshness-headers.https.html is now broken on Mac out and still partially flaky on iOS

History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fpartitioned-service-worker.tentative.https.html

diff:
--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-no-freshness-headers.https-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-no-freshness-headers.https-actual.txt
@@ -1,3 +1,3 @@
 
-PASS The headers of FetchEvent shouldn't contain freshness headers.
+FAIL The headers of FetchEvent shouldn't contain freshness headers. assert_false: if-none-match header must not be set in the FetchEvent's request. (url = https://localhost:9443/service-workers/service-worker/resources/fetch-request-no-freshness-headers-script.py) expected false got true

Alone the issue does not reproduce for LayoutTests/imported/w3c/web-platform-tests/service-
workers/service-worker/fetch-request-no-freshness-headers.https.html.
But it reproduces when running LayoutTests/imported/w3c/web-platform-tests/service-
workers/service-worker/fetch-request-no-freshness-headers.https.html and imported/w3c/web-platform-tests/service-workers/service-worker/fetch-frame-resource.https.html together.

When run alone, the service worker is in the same process as the test page and the same memory cache singleton is used. In that case, because we have a check that service worker identifiers do not match, we do not reuse the memory cache.

When the two tests are run, the service worker is in its own process and the memory cache kicks in for the web page. This adds the If-None-Match header before sending it to the service worker which will see this header.

Reopening to attach new patch.

Created attachment 454939 [details]
Patch

Committed r291485 (248599@main): <https://commits.webkit.org/248599@main>

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

*** Bug 237158 has been marked as a duplicate of this bug. ***