Bug 180702

Summary: [Service Workers] Implement "Soft Update" algorithm
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, dbates, esprehn+autocc, ews-watchlist, ggaren, jlewis3, kangil.han, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=181166
https://bugs.webkit.org/show_bug.cgi?id=181167
Bug Depends on: 179808    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-12-12 10:32:08 PST
Implement "Soft Update" algorithm:
- https://w3c.github.io/ServiceWorker/#soft-update-algorithm
Comment 1 Chris Dumez 2017-12-20 14:06:16 PST
<rdar://problem/36163461>
Comment 2 Chris Dumez 2017-12-21 10:35:11 PST
Created attachment 330036 [details]
Patch
Comment 3 Chris Dumez 2017-12-21 10:44:57 PST
Created attachment 330040 [details]
Patch
Comment 4 youenn fablet 2017-12-21 21:52:50 PST
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?
Comment 5 Chris Dumez 2017-12-22 12:44:59 PST
Created attachment 330138 [details]
Patch
Comment 6 WebKit Commit Bot 2017-12-22 13:17:51 PST
Comment on attachment 330138 [details]
Patch

Clearing flags on attachment: 330138

Committed r226274: <https://trac.webkit.org/changeset/226274>
Comment 7 WebKit Commit Bot 2017-12-22 13:17:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Matt Lewis 2017-12-26 17:41:59 PST
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
Comment 9 Matt Lewis 2017-12-26 17:54:08 PST
Created new bug for above flakiness:
https://bugs.webkit.org/show_bug.cgi?id=181166