Bug 204119 - Take service worker assertions based on client processes assertion states
Summary: Take service worker assertions based on client processes assertion states
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: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-12 09:57 PST by youenn fablet
Modified: 2021-05-10 12:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (25.17 KB, patch)
2019-11-12 10:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.32 KB, patch)
2019-11-12 11:46 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.32 KB, patch)
2019-11-12 15:17 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.50 KB, patch)
2019-11-12 17:43 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (14.34 MB, application/zip)
2019-11-13 04:53 PST, EWS Watchlist
no flags Details
Patch for landing (25.57 KB, patch)
2019-11-13 09:45 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (25.66 KB, patch)
2019-11-13 10:15 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-11-12 09:57:57 PST
Take service worker assertions based on client processes assertion states
Comment 1 youenn fablet 2019-11-12 10:04:45 PST
Created attachment 383362 [details]
Patch
Comment 2 youenn fablet 2019-11-12 10:05:47 PST
<rdar://problem/57048834>
Comment 3 youenn fablet 2019-11-12 11:46:16 PST
Created attachment 383367 [details]
Patch
Comment 4 youenn fablet 2019-11-12 15:17:18 PST
Created attachment 383392 [details]
Patch
Comment 5 youenn fablet 2019-11-12 17:43:48 PST
Created attachment 383413 [details]
Patch
Comment 6 EWS Watchlist 2019-11-13 04:53:47 PST
Comment on attachment 383413 [details]
Patch

Attachment 383413 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13246496

New failing tests:
imported/blink/fast/events/panScroll-crash.html
Comment 7 EWS Watchlist 2019-11-13 04:53:49 PST
Created attachment 383449 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 8 Chris Dumez 2019-11-13 08:14:56 PST
Comment on attachment 383413 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1239
> +    serviceWorkerProcess->registerWebProcessToServiceWorkerProcess(*webProcess);

Could this be named better? Maybe registerClientWebProcess() / unregisterClientWebProcess() ?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:65
> +    RegisterWebProcessToServiceWorkerProcess(WebCore::ProcessIdentifier webProcessIdentifier, WebCore::ProcessIdentifier serviceWorkerProcessIdentifier)

I think the naming could be better. Maybe RegisterServiceWorkerProcessClient() or RegisterServiceWorkerProcessClientWebProcess()

> Source/WebKit/UIProcess/WebProcessPool.cpp:2051
> +    callOnMainRunLoop([this, weakThis = makeWeakPtr(this)] {

What's the benefit about doing this in the next run loop iteration?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1667
> +

waitUntilServiceWorkerProcessForegroundActivityState(webView.get(), false);  to be safe?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1669
> +    waitUntilServiceWorkerProcessForegroundActivityState(webView.get(), true);

waitUntilServiceWorkerProcessBackgroundActivityState(webView.get(), false); to be safe?
Comment 9 youenn fablet 2019-11-13 08:47:07 PST
Comment on attachment 383413 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:2051
>> +    callOnMainRunLoop([this, weakThis = makeWeakPtr(this)] {
> 
> What's the benefit about doing this in the next run loop iteration?

I tried to explain this in the comment above.

WebProcessPool::updateProcessAssertions gets called at creation of the token, but the token is not yet assigned to WebProcessProxy.
If calling updateServiceWorkerProcessAssertion(), the WebProcessProxy will be seen as not holding an assertion although it will be just after.
Comment 10 youenn fablet 2019-11-13 09:28:11 PST
Let's go with registerServiceWorkerClientProcess/unregisterServiceWorkerClientProcess
Comment 11 youenn fablet 2019-11-13 09:45:44 PST
Created attachment 383463 [details]
Patch for landing
Comment 12 youenn fablet 2019-11-13 10:15:54 PST
Created attachment 383466 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-11-13 11:15:20 PST
Comment on attachment 383466 [details]
Patch for landing

Clearing flags on attachment: 383466

Committed r252418: <https://trac.webkit.org/changeset/252418>
Comment 14 WebKit Commit Bot 2019-11-13 11:15:22 PST
All reviewed patches have been landed.  Closing bug.