Summary: | Service Worker doesn't terminate after a period of time when thread blocking | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Kay <benjamin.kay> | ||||||||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Blocker | CC: | achristensen, cdumez, commit-queue, ggaren, sam, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari 12 | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | macOS 10.14 | ||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=181563 https://bugs.webkit.org/show_bug.cgi?id=202195 |
||||||||||||||
Attachments: |
|
Description
Benjamin Kay
2019-10-15 09:37:36 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. Created attachment 384503 [details]
Patch
Created attachment 384508 [details]
Patch
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). I'l rename to serviceWorkerShortTimeout. Created attachment 384598 [details]
Patch
Created attachment 384610 [details]
Patch
Why a boolean? This seems much better suited as an int or float. (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. Ping review? How does this relate to https://bugs.webkit.org/show_bug.cgi?id=181563 ? (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 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 Created attachment 386386 [details]
Rebasing
Comment on attachment 386386 [details] Rebasing Clearing flags on attachment: 386386 Committed r253898: <https://trac.webkit.org/changeset/253898> All reviewed patches have been landed. Closing bug. |