Bug 233719 – [WK2] Make Web Lock API work across multiple WebProcesses

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Make Web Lock API work across multiple WebProcesses when using modern WebKit.

[Chris Dumez]

Created attachment 445610 [details]
WIP Patch

[Chris Dumez]

Created attachment 445622 [details]
Patch

[Chris Dumez]

Created attachment 445624 [details]
Patch

[Chris Dumez]

Created attachment 445627 [details]
Patch

[Chris Dumez]

Created attachment 445629 [details]
Patch

[Chris Dumez]

Created attachment 445630 [details]
Patch

[Chris Dumez]

Created attachment 445634 [details]
Patch

[Chris Dumez]

Created attachment 445647 [details]
Patch

[Chris Dumez]

Created attachment 445668 [details]
Patch

[Chris Dumez]

Created attachment 445670 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 445727 [details]
Patch

[Chris Dumez]

Created attachment 445736 [details]
Patch

[Chris Dumez]

Created attachment 445763 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85990763>