Bug 211157

Summary:

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

Product:

WebKit

Reporter:

Alex Christensen <achristensen>

Component:

New Bugs

Assignee:

Alex Christensen <achristensen>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

beidson, cdumez, ggaren, sihui_liu

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	cdumez: commit-queue-

Alex Christensen

Reported 2020-04-28 17:18:02 PDT

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

Attachments
Patch (7.93 KB, patch) 2020-04-28 17:18 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (6.51 KB, patch) 2020-04-30 12:15 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (6.94 KB, patch) 2020-04-30 12:34 PDT, Alex Christensen	cdumez: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2020-04-28 17:18:58 PDT

Created attachment 397911 [details] Patch

Chris Dumez

Comment 2 2020-04-30 11:46:25 PDT

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 3 2020-04-30 11:56:37 PDT

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 4 2020-04-30 11:58:29 PDT

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

Comment 5 2020-04-30 12:01:13 PDT

(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 6 2020-04-30 12:06:40 PDT

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

Comment 7 2020-04-30 12:08:03 PDT

*gasp* You're right. Will fix.

Alex Christensen

Comment 8 2020-04-30 12:15:55 PDT

Created attachment 398077 [details] Patch

Chris Dumez

Comment 9 2020-04-30 12:29:34 PDT

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

Comment 10 2020-04-30 12:34:11 PDT

Created attachment 398082 [details] Patch

Chris Dumez

Comment 11 2020-04-30 12:36:55 PDT

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

Comment 12 2022-07-01 11:57:10 PDT

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 ***

Note You need to log in before you can comment on or make changes to this bug.