Remove unnecessary BinarySemaphore waits on the main thread in the network process
Created attachment 397911 [details] Patch
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?
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.
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.
(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.
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.
*gasp* You're right. Will fix.
Created attachment 398077 [details] Patch
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.
Created attachment 398082 [details] Patch
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)
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 ***