Bug 231285

Summary: Add support for processing push events without service worker clients
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, eric.carlson, ews-watchlist, glenn, hta, jer.noble, nham, philipj, sergio, tommyw, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

youenn fablet
Reported 2021-10-06 03:15:38 PDT
Add support for processing push events without service worker clients
Attachments
Patch (24.64 KB, patch)
2021-10-06 04:00 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (11.21 KB, patch)
2021-10-06 06:38 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (26.59 KB, patch)
2021-10-06 06:59 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (26.59 KB, patch)
2021-10-06 08:32 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch for landing (26.25 KB, patch)
2021-10-08 00:48 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2021-10-06 04:00:28 PDT
Created attachment 440351 [details] Patch
youenn fablet
Comment 2 2021-10-06 06:38:58 PDT
Created attachment 440359 [details] Patch
youenn fablet
Comment 3 2021-10-06 06:59:29 PDT
Created attachment 440361 [details] Patch
youenn fablet
Comment 4 2021-10-06 08:32:56 PDT
Created attachment 440372 [details] Patch
Chris Dumez
Comment 5 2021-10-07 07:16:29 PDT
Comment on attachment 440372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440372&action=review > Source/WebCore/workers/service/server/SWServer.cpp:1218 > + terminateWorkerTimer->startOneShot(weakThis && weakThis->m_isProcessTerminationDelayEnabled ? defaultTerminationDelay : 1_s); Does this mean that we have one second to: 1. IPC the SW process 2. Dispatch to the SW thread 3. Dispatch and handle the push event in JS 4. Dispatch back to the main thread 5. IPC back to the network process If so, doesn't it seem a bit low? Seems like this could cause flakiness on some slower / overloaded machines. I think our UIProcess responsiveness timer uses 3 seconds and even then, is only enabled on release builds (No debug builds, no ASAN, not under debugger).
youenn fablet
Comment 6 2021-10-07 08:17:21 PDT
(In reply to Chris Dumez from comment #5) > Comment on attachment 440372 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440372&action=review > > > Source/WebCore/workers/service/server/SWServer.cpp:1218 > > + terminateWorkerTimer->startOneShot(weakThis && weakThis->m_isProcessTerminationDelayEnabled ? defaultTerminationDelay : 1_s); > > Does this mean that we have one second to: > 1. IPC the SW process > 2. Dispatch to the SW thread > 3. Dispatch and handle the push event in JS > 4. Dispatch back to the main thread > 5. IPC back to the network process > > If so, doesn't it seem a bit low? Seems like this could cause flakiness on > some slower / overloaded machines. Maybe, this is for testing, and this should only kick in for API tests since otherwise we will have a service worker client. Also, the only test is actually trying to trigger the timeout, so even in case we are slow and it takes more than 1 second, we should be good for that particular test. I can increase it to 3 seconds, but then the test will run for longer. Let's try it
youenn fablet
Comment 7 2021-10-08 00:48:09 PDT
Created attachment 440581 [details] Patch for landing
youenn fablet
Comment 8 2021-10-08 00:48:32 PDT
Went with 2 seconds for now.
EWS
Comment 9 2021-10-08 03:02:57 PDT
Committed r283796 (242689@main): <https://commits.webkit.org/242689@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440581 [details].
Radar WebKit Bug Importer
Comment 10 2021-10-08 03:03:17 PDT
<rdar://problem/84020871>
Note You need to log in before you can comment on or make changes to this bug.