Bug 230649 - Add support for running service workers on the main thread
Summary: Add support for running service workers on the main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 231045
  Show dependency treegraph
 
Reported: 2021-09-22 16:06 PDT by Chris Dumez
Modified: 2021-09-30 16:28 PDT (History)
19 users (show)

See Also:


Attachments
WIP patch (34.42 KB, patch)
2021-09-22 16:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (84.77 KB, patch)
2021-09-23 09:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (85.32 KB, patch)
2021-09-27 10:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (85.93 KB, patch)
2021-09-29 15:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-09-22 16:06:57 PDT
Add support for running service workers on the main thread.
Comment 1 Chris Dumez 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.
Comment 2 Chris Dumez 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.
Comment 3 Chris Dumez 2021-09-23 09:17:56 PDT
Created attachment 439051 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-09-23 10:08:10 PDT
<rdar://problem/83453190>
Comment 5 Sam Weinig 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.
Comment 6 Chris Dumez 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.
Comment 7 Chris Dumez 2021-09-27 10:10:33 PDT
Created attachment 439366 [details]
Patch
Comment 8 Chris Dumez 2021-09-29 13:47:23 PDT
Ping review? I need this to actually start adding support for service worker scripting from injected bundle.
Comment 9 Alex Christensen 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.
Comment 10 Chris Dumez 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.
Comment 11 Chris Dumez 2021-09-29 15:27:30 PDT
Created attachment 439669 [details]
Patch
Comment 12 EWS 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].