Bug 237160 – REGRESSION(r290356-r290351?): [ iOS EWS ] 3 imported/w3c/web-platform-tests/service-workers/service-worker/* tests are constant text failures.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dawn Morningstar]

imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-no-freshness-headers.https.html 
imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker.tentative.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/partitioned-service-worker-getRegistrations.tentative.https.html

HISTORY:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Ffetch-request-no-freshness-headers.https.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fpartitioned-service-worker-getRegistrations.tentative.https.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fpartitioned-service-worker.tentative.https.html

[Radar WebKit Bug Importer]

<rdar://problem/89440067>

[Dawn Morningstar]

Failure was reproducible at iOS15 Production TOT - r290435.

Regression range appears to be r290356-r290351.

[Dawn Morningstar]

Marking expect

[Dawn Morningstar]

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

Marking expectations since these tests are affecting ews negatively.*

[Dawn Morningstar]

Created attachment 453143 [details]
expectations

[Robert Jenner]

Comment on attachment 453143 [details]
expectations

Clearing flags on attachment: 453143

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

[Robert Jenner]

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

[Robert Jenner]

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

[Robert Jenner]

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.

[youenn fablet]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 453650 [details]
WIP patch

[Chris Dumez]

Created attachment 453662 [details]
Patch

[Chris Dumez]

Created attachment 453663 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

Comment on attachment 453663 [details]
Patch

Let’s discuss the shared workers separately.

[EWS]

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

[Chris Dumez]

Created attachment 453733 [details]
Patch

[EWS]

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].

[Truitt Savell]

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

[youenn fablet]

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.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 454939 [details]
Patch

[EWS]

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].

[Brent Fulgham]

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