Bug 233789 - Introduce a MainThreadBridge class inside WebLockManager
Summary: Introduce a MainThreadBridge class inside WebLockManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 232436
  Show dependency treegraph
 
Reported: 2021-12-02 16:23 PST by Chris Dumez
Modified: 2021-12-02 18:55 PST (History)
6 users (show)

See Also:


Attachments
Patch (19.11 KB, patch)
2021-12-02 16:26 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-12-02 16:23:20 PST
Extract all the logic to hop to and back from the main thread out of the WebLockManager and into a MainThreadBridge internal class. This is similar to what was done for BroadcastChannel already.
It provides better structuring and slightly better performance by calling isolatedCopy() on the ClientOrigin only once instead of on every call.
Comment 1 Chris Dumez 2021-12-02 16:26:12 PST
Created attachment 445786 [details]
Patch
Comment 2 Darin Adler 2021-12-02 16:54:24 PST
Comment on attachment 445786 [details]
Patch

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

> Source/WebCore/Modules/web-locks/WebLockManager.cpp:68
> +class WebLockManager::MainThreadBridge : public ThreadSafeRefCounted<MainThreadBridge, WTF::DestructionThread::Main> {

Are we reference counting this just for the "destroy on main thread" bit? Because if so, I suggest we instead use unique_ptr and null it out on the main thread in ~WebLockManager. I don’t see any ref/deref of this going on.
Comment 3 Chris Dumez 2021-12-02 16:55:39 PST
Comment on attachment 445786 [details]
Patch

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

We need ref-counting to keep ourselves alive while dispatching to the main thread as well.

> Source/WebCore/Modules/web-locks/WebLockManager.cpp:161
> +            Ref protectedThis { *this };

We Ref here.

> Source/WebCore/Modules/web-locks/WebLockManager.cpp:165
> +        downcast<WorkerGlobalScope>(*m_context).thread().workerLoaderProxy().postTaskToLoader([task = WTFMove(task), protectedThis = Ref { *this }](auto& context) {

and here.
Comment 4 EWS 2021-12-02 18:54:46 PST
Committed r286474 (244813@main): <https://commits.webkit.org/244813@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445786 [details].
Comment 5 Radar WebKit Bug Importer 2021-12-02 18:55:25 PST
<rdar://problem/85999405>