Bug 211157 – Remove unnecessary BinarySemaphore waits on the main thread in the network process

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Alex Christensen]

Remove unnecessary BinarySemaphore waits on the main thread in the network process

[Alex Christensen]

Created attachment 397911 [details]
Patch

[Chris Dumez]

Comment on attachment 397911 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:124
> +    m_queue->dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {

How is capturing |this| safe here?

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:131
> +        RunLoop::main().dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {

How is capturing |this| safe here?

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:146
> +    m_queue->dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {

How is capturing |this| safe here?

[Alex Christensen]

Comment on attachment 397911 [details]
Patch

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

>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:146
>> +    m_queue->dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {
> 
> How is capturing |this| safe here?

It's just as safe as it was.  I had the same concern so looked into the lifetime, and it turns out this lives as long as the NetworkProcess, which is a singleton which is never destroyed.

[Chris Dumez]

Comment on attachment 397911 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:146
>>> +    m_queue->dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {
>> 
>> How is capturing |this| safe here?
> 
> It's just as safe as it was.  I had the same concern so looked into the lifetime, and it turns out this lives as long as the NetworkProcess, which is a singleton which is never destroyed.

> It's just as safe as it was.

Not convinced this is a true statement. We used to hang the main thread while the task was getting processed on the background thread, and the StorageManagerSet only gets destroyed on the main thread.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 397911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397911&action=review
> 
> >>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:146
> >>> +    m_queue->dispatch([this, completionHandler = WTFMove(completionHandler)] () mutable {
> >> 
> >> How is capturing |this| safe here?
> > 
> > It's just as safe as it was.  I had the same concern so looked into the lifetime, and it turns out this lives as long as the NetworkProcess, which is a singleton which is never destroyed.
> 
> > It's just as safe as it was.
> 
> Not convinced this is a true statement. We used to hang the main thread
> while the task was getting processed on the background thread, and the
> StorageManagerSet only gets destroyed on the main thread.

The argument about it living as long as the NetworkProcess is valid though. I am looking into this more so I am 100% sure it is still safe, despite the behavior change.

[Chris Dumez]

Comment on attachment 397911 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h:-50
> -    static Ref<StorageManagerSet> create();

WorkQueueMessageReceiver is ThreadSafeRefCounted and gets ref'd/deref'd by the Connection. Dropping the factory to use UniqueRef does not seem right.

[Alex Christensen]

*gasp*
You're right.  Will fix.

[Alex Christensen]

Created attachment 398077 [details]
Patch

[Chris Dumez]

Comment on attachment 398077 [details]
Patch

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

r=me with fix

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:157
> +        RunLoop::main().dispatch(WTFMove(completionHandler));

You need to WTFMove(protectedThis) too or you may cause StorageManagerSet to get destroyed on the background queue, which would hit the assertion in the destructor.

[Alex Christensen]

Created attachment 398082 [details]
Patch

[Chris Dumez]

Comment on attachment 398082 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:157
> +        RunLoop::main().dispatch(WTFMove(completionHandler));

WTFMove(protectedThis)

[Alex Christensen]

This code was removed in https://bugs.webkit.org/show_bug.cgi?id=238938

*** This bug has been marked as a duplicate of bug 238938 ***