Bug 230649 – Add support for running service workers on the main thread

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

[Chris Dumez]

Add support for running service workers on the main thread.

[Chris Dumez]

Created attachment 438988 [details]
WIP patch

Forces all service workers on the main thread for now to see if EWS is happy.

[Chris Dumez]

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]

Created attachment 439051 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83453190>

[Sam Weinig]

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]

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

Created attachment 439366 [details]
Patch

[Chris Dumez]

Ping review? I need this to actually start adding support for service worker scripting from injected bundle.

[Alex Christensen]

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 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]

Created attachment 439669 [details]
Patch

[EWS]

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