Bug 183873

Summary: Promptly terminate service worker processes when they are no longer needed
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 183941    
Bug Blocks:    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
Patch
none
Patch none

Chris Dumez
Reported 2018-03-21 15:00:02 PDT
Promptly terminate service worker processes when they are no longer needed.
Attachments
WIP patch (12.56 KB, patch)
2018-03-22 13:19 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP patch (14.46 KB, patch)
2018-03-22 13:53 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP patch (16.91 KB, patch)
2018-03-22 16:25 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP patch (25.61 KB, patch)
2018-03-22 16:58 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (34.84 KB, patch)
2018-03-23 10:08 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (32.81 KB, patch)
2018-03-23 14:36 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2018-03-21 15:00:37 PDT
<rdar://problem/38676995>
Chris Dumez
Comment 2 2018-03-22 13:19:14 PDT
Created attachment 336301 [details] WIP patch
Chris Dumez
Comment 3 2018-03-22 13:53:30 PDT
Created attachment 336309 [details] WIP patch
Chris Dumez
Comment 4 2018-03-22 16:25:39 PDT
Created attachment 336324 [details] WIP patch
Chris Dumez
Comment 5 2018-03-22 16:58:53 PDT
Created attachment 336337 [details] WIP patch
Chris Dumez
Comment 6 2018-03-23 10:08:36 PDT
Created attachment 336384 [details] Patch
youenn fablet
Comment 7 2018-03-23 11:58:23 PDT
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 8 2018-03-23 12:40:46 PDT
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
Comment 9 2018-03-23 14:36:01 PDT
Created attachment 336423 [details] Patch
WebKit Commit Bot
Comment 10 2018-03-23 15:16:29 PDT
Comment on attachment 336423 [details] Patch Clearing flags on attachment: 336423 Committed r229927: <https://trac.webkit.org/changeset/229927>
WebKit Commit Bot
Comment 11 2018-03-23 15:16:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.