Bug 183873 – Promptly terminate service worker processes when they are no longer needed

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

[Chris Dumez]

Promptly terminate service worker processes when they are no longer needed.

[Chris Dumez]

<rdar://problem/38676995>

[Chris Dumez]

Created attachment 336301 [details]
WIP patch

[Chris Dumez]

Created attachment 336309 [details]
WIP patch

[Chris Dumez]

Created attachment 336324 [details]
WIP patch

[Chris Dumez]

Created attachment 336337 [details]
WIP patch

[Chris Dumez]

Created attachment 336384 [details]
Patch

[youenn fablet]

Comment on attachment 336384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336384&action=review

> Source/WebKit/ChangeLog:37
> +        prevent the service worker process from ever being terminated.

There is no need for this once https://bugs.webkit.org/show_bug.cgi?id=183941 lands.
We should ask for registration of service worker clients at creation of the first service worker process now that we have more than one.
We could add an assertion in WebProcess::registerServiceWorkerClients to ensure that this is not a service worker process getting this message for safety.

[youenn fablet]

Comment on attachment 336384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336384&action=review

> Source/WebCore/workers/service/server/SWServer.cpp:745
> +    auto& clientIdentifiersForSecurityOrigin = m_clientsBySecurityOrigin.ensure(WTFMove(securityOrigin), [] {

Would it be more efficient and simpler if m_clientsBySecurityOrigin takes a SecurityOriginData?
It is unfortunate that we would need to create all these Ref<SecurityOrigin> just for querying this map.
I guess there are other places that would be affected by this change.

> Source/WebCore/workers/service/server/SWServer.cpp:789
> +        if (m_shouldDisableServiceWorkerProcessTerminationDelay)

It might be better to keep the idea of a timer for terminating workers even in the context of API tests.
This would make it closer to actual WebKit behavior.
Can we instead do startOneShort(m_shouldDisableServiceWorkerProcessTerminationDelay ? 0 : terminationDelay); ?

Or maybe, since we have this piping from UIProcess to WebProcess, we could pass the timer value itself in case we want at some point to give application the ability to set this value.

> Source/WebCore/workers/service/server/SWServer.cpp:801
> +    clientsForSecurityOrigin.removeFirstMatching([&] (const auto& identifier) {

Would it be better if it was a HashSet. We probably do not need to keep the order here.
Maybe we can also assert that we are actually finding this clientIdentifier.

> Source/WebKit/StorageProcess/StorageProcess.cpp:134
> +    return false;

I wonder whether WTF::anyOf(m_swServers.values(), [&]...) would work here.

[Chris Dumez]

Created attachment 336423 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336423 [details]
Patch

Clearing flags on attachment: 336423

Committed r229927: <https://trac.webkit.org/changeset/229927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.