Summary: | Crash under WebProcessProxy::shouldSendPendingMessage() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, ddkilzer, ews-watchlist, ggaren, jiewen_tan, kkinnunen, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222992 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 224448 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2021-04-09 10:24:35 PDT
Created attachment 425630 [details]
Patch
Comment on attachment 425630 [details]
Patch
Actually, sendMessage() relies on state() so it is still not thread-safe. I'll rework this.
Created attachment 425644 [details]
Patch
Comment on attachment 425644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425644&action=review r=me > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:254 > - auto encoder = WTFMove(pendingMessage.encoder); > auto sendOptions = pendingMessage.sendOptions; > if (pendingMessage.asyncReplyInfo) > IPC::addAsyncReplyHandler(*connection(), pendingMessage.asyncReplyInfo->second, WTFMove(pendingMessage.asyncReplyInfo->first)); > - m_connection->sendMessage(WTFMove(encoder), sendOptions); > + m_connection->sendMessage(WTFMove(pendingMessage.encoder), sendOptions); Super nit: Why not just inline `sendOptions` as well if you're inlining `encoder`? Or move it after the if statement? Not necessary to change to land, though. Comment on attachment 425644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425644&action=review >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:254 >> + m_connection->sendMessage(WTFMove(pendingMessage.encoder), sendOptions); > > Super nit: Why not just inline `sendOptions` as well if you're inlining `encoder`? Or move it after the if statement? Not necessary to change to land, though. Oh, maybe IPC::addAsyncReplyHandler() alters `sendOptions`? Created attachment 425690 [details]
Patch
(In reply to David Kilzer (:ddkilzer) from comment #5) > Comment on attachment 425644 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425644&action=review > > r=me > > > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:254 > > - auto encoder = WTFMove(pendingMessage.encoder); > > auto sendOptions = pendingMessage.sendOptions; > > if (pendingMessage.asyncReplyInfo) > > IPC::addAsyncReplyHandler(*connection(), pendingMessage.asyncReplyInfo->second, WTFMove(pendingMessage.asyncReplyInfo->first)); > > - m_connection->sendMessage(WTFMove(encoder), sendOptions); > > + m_connection->sendMessage(WTFMove(pendingMessage.encoder), sendOptions); > > Super nit: Why not just inline `sendOptions` as well if you're inlining > `encoder`? Or move it after the if statement? Not necessary to change to > land, though. I made the change. I see no good reason why this couldn't be inlined. Committed r275805 (236376@main): <https://commits.webkit.org/236376@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425690 [details]. |