Bug 202992 - Service Worker doesn't terminate after a period of time when thread blocking
Summary: Service Worker doesn't terminate after a period of time when thread blocking
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Safari 12
Hardware: Mac macOS 10.14
: P2 Blocker
Assignee: youenn fablet
Keywords: InRadar
Depends on:
Reported: 2019-10-15 09:37 PDT by Benjamin Kay
Modified: 2019-12-24 06:19 PST (History)
7 users (show)

See Also:

Patch (50.82 KB, patch)
2019-11-29 06:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (50.91 KB, patch)
2019-11-29 07:40 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (3.40 MB, patch)
2019-12-02 01:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (50.98 KB, patch)
2019-12-02 04:42 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (54.52 KB, patch)
2019-12-24 03:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Kay 2019-10-15 09:37:36 PDT
Steps to reproduce:

We (BBC) are investigating service workers and trying to put them into an unrecoverable state. The only way we found is listed below but is a blocker to us rolling out service workers to our users until we know it can always be recovered.

We gave a service worker an infinite loop outside the event handlers, it locked up the service worker thread, blocking calls out to check for updates and essentially killed all service worker functionality with no easy way to recover as a user.

Please see https://static-misc-2.glitch.me/blocking-sw/ that was made by Jake Archibald from Google as an example where the service worker served never resolves the promise to install and blocks the thread using a while (true).

Actual results:

Service worker is blocked, and cannot update. even on a client refresh.

Expected results:

If you visit https://static-misc-2.glitch.me/blocking-sw/ on Chrome, Chrome kills the thread after 60 seconds unless dev tools is open.

I would expect Safari kill the service worker thread after a timeout period.
Comment 1 Radar WebKit Bug Importer 2019-10-15 10:53:48 PDT
Comment 2 youenn fablet 2019-10-15 12:00:30 PDT
Agreed that we should add such protections for every event, install, activate, fetch, message...
And we probably want to soft update when a sw exhibits that behavior.
Comment 3 youenn fablet 2019-11-29 06:44:15 PST
Created attachment 384503 [details]
Comment 4 youenn fablet 2019-11-29 07:40:50 PST
Created attachment 384508 [details]
Comment 5 Sam Weinig 2019-12-01 16:17:02 PST
Comment on attachment 384508 [details]

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

> Source/WebCore/page/Settings.yaml:915
> +serviceWorkerTestMode:
> +  initial: false

We generally try to avoid test modes when we can, as they don't scale. In this case, it seems like you could easily avoid it by making this setting be serviceWorkerTimeout (or something like that).
Comment 6 youenn fablet 2019-12-02 01:04:06 PST
I'l rename to serviceWorkerShortTimeout.
Comment 7 youenn fablet 2019-12-02 01:04:22 PST
Created attachment 384598 [details]
Comment 8 youenn fablet 2019-12-02 04:42:24 PST
Created attachment 384610 [details]
Comment 9 Sam Weinig 2019-12-02 07:29:38 PST
Why a boolean? This seems much better suited as an int or float.
Comment 10 youenn fablet 2019-12-02 07:56:25 PST
(In reply to Sam Weinig from comment #9)
> Why a boolean? This seems much better suited as an int or float.

This keeps the actual values close to where they are used in the code base.
A single value is hopefully sufficient to all environments except for testing.
Comment 11 youenn fablet 2019-12-04 05:43:34 PST
Ping review?
Comment 12 Chris Dumez 2019-12-05 10:35:11 PST
How does this relate to https://bugs.webkit.org/show_bug.cgi?id=181563 ?
Comment 13 youenn fablet 2019-12-06 00:51:34 PST
(In reply to Chris Dumez from comment #12)
> How does this relate to https://bugs.webkit.org/show_bug.cgi?id=181563 ?

This complements the protection added in bug 181562.
In bug 181563, we make sure that stopping a worker will actually work after a reasonable amount of time. But this will kick in only when all service worker clients or gone.
Bug 181563 does not protect service workers that are spinning at install time or when receiving a postMessage.
We added some protection around fetch, but we have no protection if the service worker is not activating/activated or does not register a fetch event handler.

This patch adds this protection and relates to https://w3c.github.io/ServiceWorker/#service-worker-lifetime.
'''A user agent may terminate service workers at any time it: Detects abnormal operation: such as infinite loops and tasks exceeding imposed time limits (if any) while handling the events.'''

One case is if a service worker is spinning at install time.
In that case, the job queue is stuck and there is no way for the web site to unblock the job queue except closing all clients, kicking in the logic to stop the service worker and open a new client after the service worker is stopped.
Comment 14 Chris Dumez 2019-12-09 16:14:53 PST
Comment on attachment 384610 [details]

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

r=me with changes

> Source/WebCore/workers/service/context/SWContextManager.h:72
> +        bool useShortTimeout() const { return m_useShortTimeout; }

usesShortTimeout() or shouldUseShortTimeout(), it is confusing otherwise as it looks like it may be a setter.

> Source/WebCore/workers/service/context/SWContextManager.h:76
> +        void setUseShortTimeout(bool value) { m_useShortTimeout = value; }

Ditto about naming.

> Source/WebCore/workers/service/context/SWContextManager.h:80
> +        bool m_useShortTimeout { false };

Ditto about naming.

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:346
> +WK_EXPORT bool WKPreferencesGetServiceWorkerShortTimeout(WKPreferencesRef preferencesRef);

Name is confusing for a boolean

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:347
> +WK_EXPORT void WKPreferencesSetServiceWorkerShortTimeout(WKPreferencesRef preferencesRef, bool enabled);

Name is confusing for a boolean
Comment 15 youenn fablet 2019-12-24 03:27:30 PST
Created attachment 386386 [details]
Comment 16 WebKit Commit Bot 2019-12-24 06:19:53 PST
Comment on attachment 386386 [details]

Clearing flags on attachment: 386386

Committed r253898: <https://trac.webkit.org/changeset/253898>
Comment 17 WebKit Commit Bot 2019-12-24 06:19:55 PST
All reviewed patches have been landed.  Closing bug.