[webkit-reviews] review granted: [Bug 180702] [Service Workers] Implement "Soft Update" algorithm : [Attachment 330040] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 180702: [Service Workers] Implement "Soft Update" algorithm
https://bugs.webkit.org/show_bug.cgi?id=180702

Attachment 330040: Patch

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




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 330040
  --> https://bugs.webkit.org/attachment.cgi?id=330040
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?