Bug 202992 – Service Worker doesn't terminate after a period of time when thread blocking

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Benjamin Kay]

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.

[Radar WebKit Bug Importer]

<rdar://problem/56298596>

[youenn fablet]

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.

[youenn fablet]

Created attachment 384503 [details]
Patch

[youenn fablet]

Created attachment 384508 [details]
Patch

[Sam Weinig]

Comment on attachment 384508 [details]
Patch

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).

[youenn fablet]

I'l rename to serviceWorkerShortTimeout.

[youenn fablet]

Created attachment 384598 [details]
Patch

[youenn fablet]

Created attachment 384610 [details]
Patch

[Sam Weinig]

Why a boolean? This seems much better suited as an int or float.

[youenn fablet]

(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.

[youenn fablet]

Ping review?

[Chris Dumez]

How does this relate to https://bugs.webkit.org/show_bug.cgi?id=181563 ?

[youenn fablet]

(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.

[Chris Dumez]

Comment on attachment 384610 [details]
Patch

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

[youenn fablet]

Created attachment 386386 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 386386 [details]
Rebasing

Clearing flags on attachment: 386386

Committed r253898: <https://trac.webkit.org/changeset/253898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.