Bug 180702 - [Service Workers] Implement "Soft Update" algorithm
Summary: [Service Workers] Implement "Soft Update" algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 179808
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-12 10:32 PST by Chris Dumez
Modified: 2017-12-26 18:13 PST (History)
10 users (show)

See Also:


Attachments
Patch (39.49 KB, patch)
2017-12-21 10:35 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.83 KB, patch)
2017-12-21 10:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.99 KB, patch)
2017-12-22 12:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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