Bug 180702 – [Service Workers] Implement "Soft Update" algorithm

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

Implement "Soft Update" algorithm:
- https://w3c.github.io/ServiceWorker/#soft-update-algorithm

[Chris Dumez]

<rdar://problem/36163461>

[Chris Dumez]

Created attachment 330036 [details]
Patch

[Chris Dumez]

Created attachment 330040 [details]
Patch

[youenn fablet]

Comment on attachment 330040 [details]
Patch

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

> Source/WebCore/workers/service/SWClientConnection.cpp:132
> +        failedFetchingScript(jobDataIdentifier, registrationKey, ResourceError { errorDomainWebKitInternal, 0, URL(), ASCIILiteral("Failed to fetch service worker script") });

I don't know whether this makeScopeExit is worth it here:
- There is only one early return
- We could probably have different error messages for the two failure cases.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:195
>      if (!context || !context->sessionID().isValid()) {

Given updateRegistration is only called by ServiceWorkerRegistration, we probably do not need to check that context is null since this is done at the call sites.
We could also move that if statement and maybe the next one to the two call sites.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:368
>          context->postTask([job = makeRef(job), exception](ScriptExecutionContext&) {

Looking at the code, it seems we take a job ref both to go to main thread and to go to worker thread.
Given it carries a Ref<DeferredPromise>, I do not think this is actually safe, since we may destroy the promise in the main thread.
We should probably rework this. For instance m_scheduledJobs should not have refs of jobs that are created in the worker thread.

> Source/WebCore/workers/service/context/SWContextManager.cpp:131
> +    serviceWorker->thread().runLoop().postTask([task = WTFMove(task)] (auto& context) {

Are we sure postTask will actually always execute the task?
I would guess that if we are trying to terminate the worker, adding a task in the queue will not make the task be executed.
Maybe the m_workerMap check prevents that case. If so, we could assert that the queue is not being killed.

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:141
> +    if (isNonSubresourceRequest || (registration.lastUpdateTime() && (WallTime::now() - registration.lastUpdateTime()) > 86400_s))

We probably have this now - lastUpdateTime > 86400 in various places.
Would it be useful to have a helper function for this on the registration?

[Chris Dumez]

Created attachment 330138 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330138 [details]
Patch

Clearing flags on attachment: 330138

Committed r226274: <https://trac.webkit.org/changeset/226274>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

After this patch the test imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html has become very flaky on WK2. It is most common on macOS:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fregister-same-scope-different-script-url.https.html

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r226294%20(1953)/results.html
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/1953


diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-actual.txt
@@ -1,7 +1,9 @@
+
+Harness Error (TIMEOUT), message = null
 
 PASS Register different scripts concurrently 
 PASS Register then register new script URL 
 PASS Register then register new script URL that 404s 
 FAIL Register then register new script that does not install assert_unreached: unexpected rejection: assert_equals: on redundant, installing should be null expected null but got object "[object ServiceWorker]" Reached unreachable code
-PASS Register same-scope new script url effect on controller 
+TIMEOUT Register same-scope new script url effect on controller Test timed out

[Matt Lewis]

Created new bug for above flakiness:
https://bugs.webkit.org/show_bug.cgi?id=181166