Bug 233719 - [WK2] Make Web Lock API work across multiple WebProcesses
Summary: [WK2] Make Web Lock API work across multiple WebProcesses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 233289
Blocks: 232436
  Show dependency tree / graph
 
Reported: 2021-12-01 13:07 PST by Chris Dumez
Modified: 2021-12-02 15:00 PST (History)
15 users (show)

See Also:

Attachments
WIP Patch (93.15 KB, patch)
2021-12-01 13:44 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.25 KB, patch)
2021-12-01 15:16 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.34 KB, patch)
2021-12-01 15:27 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.37 KB, patch)
2021-12-01 15:42 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.88 KB, patch)
2021-12-01 16:11 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (119.91 KB, patch)
2021-12-01 16:14 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (120.63 KB, patch)
2021-12-01 16:51 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (120.63 KB, patch)
2021-12-01 17:32 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (120.66 KB, patch)
2021-12-01 21:34 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (121.23 KB, patch)
2021-12-01 21:46 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (122.49 KB, patch)
2021-12-02 09:29 PST, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (122.48 KB, patch)
2021-12-02 10:25 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (123.18 KB, patch)
2021-12-02 12:46 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (12) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-12-01 13:07:30 PST 
Make Web Lock API work across multiple WebProcesses when using modern WebKit.
Comment 1 Chris Dumez 2021-12-01 13:44:40 PST 
Created attachment 445610 [details]
WIP Patch
Comment 2 Chris Dumez 2021-12-01 15:16:58 PST 
Created attachment 445622 [details]
Patch
Comment 3 Chris Dumez 2021-12-01 15:27:56 PST 
Created attachment 445624 [details]
Patch
Comment 4 Chris Dumez 2021-12-01 15:42:41 PST 
Created attachment 445627 [details]
Patch
Comment 5 Chris Dumez 2021-12-01 16:11:38 PST 
Created attachment 445629 [details]
Patch
Comment 6 Chris Dumez 2021-12-01 16:14:23 PST 
Created attachment 445630 [details]
Patch
Comment 7 Chris Dumez 2021-12-01 16:51:29 PST 
Created attachment 445634 [details]
Patch
Comment 8 Chris Dumez 2021-12-01 17:32:28 PST 
Created attachment 445647 [details]
Patch
Comment 9 Chris Dumez 2021-12-01 21:34:52 PST 
Created attachment 445668 [details]
Patch
Comment 10 Chris Dumez 2021-12-01 21:46:51 PST 
Created attachment 445670 [details]
Patch
Comment 11 Alex Christensen 2021-12-02 08:18:03 PST 
Comment on attachment 445670 [details]
Patch

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

Would it be worth adding a test using _relatedWebView to verify what happens when you have two pages in the same process?

> Source/WebCore/Modules/web-locks/WebLockRegistry.h:76
> +    HashMap<ClientOrigin, PerOriginRegistry*> m_perOriginRegistries;

Can this be a weak pointer?

> Source/WebKit/WebProcess/WebCoreSupport/RemoteWebLockRegistry.messages.in:28
> +    DidCompleteLockRequest(WebCore::WebLockIdentifier lockIdentifier, WebCore::ScriptExecutionContextIdentifier clientID, bool success);
> +    DidStealLock(WebCore::WebLockIdentifier lockIdentifier, WebCore::ScriptExecutionContextIdentifier clientID);

It seems like this can be a completion handler on the RequestLock message that just contains whether it was success, failure, or stolen.  That would be nicer than introducing a new messages.in just for replies.
Comment 12 Chris Dumez 2021-12-02 08:34:14 PST 
(In reply to Alex Christensen from comment #11)
> Comment on attachment 445670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445670&action=review
> 
> Would it be worth adding a test using _relatedWebView to verify what happens
> when you have two pages in the same process?
> 
> > Source/WebCore/Modules/web-locks/WebLockRegistry.h:76
> > +    HashMap<ClientOrigin, PerOriginRegistry*> m_perOriginRegistries;
> 
> Can this be a weak pointer?

It can. It is not strictly needed but it is always better to be safe. I'll make the change.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/RemoteWebLockRegistry.messages.in:28
> > +    DidCompleteLockRequest(WebCore::WebLockIdentifier lockIdentifier, WebCore::ScriptExecutionContextIdentifier clientID, bool success);
> > +    DidStealLock(WebCore::WebLockIdentifier lockIdentifier, WebCore::ScriptExecutionContextIdentifier clientID);
> 
> It seems like this can be a completion handler on the RequestLock message
> that just contains whether it was success, failure, or stolen.  That would
> be nicer than introducing a new messages.in just for replies.

There are several issues with that:
1. It could get called twice: once with success because we got the lock and a second time later on because our lock got stolen.
2. It may not get called at all (request may get aborted before it is processed)

Both are really compatible with CompletionHandler and this is why the API currently uses 2 Functions.
Comment 13 Chris Dumez 2021-12-02 09:29:46 PST 
Created attachment 445727 [details]
Patch
Comment 14 Chris Dumez 2021-12-02 10:25:36 PST 
Created attachment 445736 [details]
Patch
Comment 15 Chris Dumez 2021-12-02 12:46:58 PST 
Created attachment 445763 [details]
Patch
Comment 16 EWS 2021-12-02 14:59:47 PST 
Committed r286455 (244797@main): <https://commits.webkit.org/244797@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445763 [details].
Comment 17 Radar WebKit Bug Importer 2021-12-02 15:00:39 PST 
<rdar://problem/85990763>