Bug 230649

Summary: Add support for running service workers on the main thread
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, annulen, beidson, ews-watchlist, ggaren, gyuyoung.kim, hi, japhet, joepeck, jsbell, pangle, ryuan.choi, sam, sergio, toyoshim, webkit-bug-importer, youennf, yutak
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231045    
Attachments:
Description Flags
WIP patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2021-09-22 16:06:57 PDT
Add support for running service workers on the main thread.
Attachments
WIP patch (34.42 KB, patch)
2021-09-22 16:22 PDT, Chris Dumez
no flags
Patch (84.77 KB, patch)
2021-09-23 09:17 PDT, Chris Dumez
no flags
Patch (85.32 KB, patch)
2021-09-27 10:10 PDT, Chris Dumez
no flags
Patch (85.93 KB, patch)
2021-09-29 15:27 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2021-09-22 16:22:27 PDT
Created attachment 438988 [details] WIP patch Forces all service workers on the main thread for now to see if EWS is happy.
Chris Dumez
Comment 2 2021-09-23 07:18:04 PDT
Good, only 2 test failures and they are tests about hung service workers (and their termination) and this is obviously not working as well if the service worker runs on the main thread.
Chris Dumez
Comment 3 2021-09-23 09:17:56 PDT
Radar WebKit Bug Importer
Comment 4 2021-09-23 10:08:10 PDT
Sam Weinig
Comment 5 2021-09-24 11:46:14 PDT
Comment on attachment 439051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439051&action=review > Source/WebCore/ChangeLog:8 > + Add support for running service workers on the main thread (off by default). Can you expand on the change log here and add some information about what the goal is here.
Chris Dumez
Comment 6 2021-09-27 09:59:38 PDT
(In reply to Sam Weinig from comment #5) > Comment on attachment 439051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439051&action=review > > > Source/WebCore/ChangeLog:8 > > + Add support for running service workers on the main thread (off by default). > > Can you expand on the change log here and add some information about what > the goal is here. Ok, I will add more information. Basically, the idea is to support uses where the injected bundle needs to be able to gets access to the service worker global object and script it. This is not practical if the service worker runs off the main thread. Note that this is not for service workers in general. It will be only for very specific service workers that need to interact with the injected bundle.
Chris Dumez
Comment 7 2021-09-27 10:10:33 PDT
Chris Dumez
Comment 8 2021-09-29 13:47:23 PDT
Ping review? I need this to actually start adding support for service worker scripting from injected bundle.
Alex Christensen
Comment 9 2021-09-29 13:56:45 PDT
Comment on attachment 439366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439366&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2492 > + return makeUnique<SWServer>(makeUniqueRef<WebSWOriginStore>(), info.processTerminationDelayEnabled, WTFMove(path), sessionID, workerThreadMode, parentProcessHasServiceWorkerEntitlement(), [this, sessionID](auto&& jobData, bool shouldRefreshCache, auto&& request, auto&& completionHandler) mutable { Not new to this patch, but I don't think we need the unsafe capture of this here. Below we could also just capture a RefPtr<Connection> instead of this. > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.mm:489 > +- (BOOL)shouldRunServiceWorkersOnMainThread ForTesting would be nice on this here and everywhere else.
Chris Dumez
Comment 10 2021-09-29 14:00:09 PDT
Comment on attachment 439366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439366&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2492 >> + return makeUnique<SWServer>(makeUniqueRef<WebSWOriginStore>(), info.processTerminationDelayEnabled, WTFMove(path), sessionID, workerThreadMode, parentProcessHasServiceWorkerEntitlement(), [this, sessionID](auto&& jobData, bool shouldRefreshCache, auto&& request, auto&& completionHandler) mutable { > > Not new to this patch, but I don't think we need the unsafe capture of this here. Below we could also just capture a RefPtr<Connection> instead of this. NetworkProcess owns the SWSessions and SWSession is not ref-counted. Therefore, capturing |this| seems safe to me? >> Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.mm:489 >> +- (BOOL)shouldRunServiceWorkersOnMainThread > > ForTesting would be nice on this here and everywhere else. Good idea.
Chris Dumez
Comment 11 2021-09-29 15:27:30 PDT
EWS
Comment 12 2021-09-29 19:20:09 PDT
Committed r283295 (242320@main): <https://commits.webkit.org/242320@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439669 [details].
Note You need to log in before you can comment on or make changes to this bug.