Crash under WebProcessProxy::shouldSendPendingMessage(): ``` Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000000) [ 0] 0x0000000190ba03cc WebKit`WebKit::WebProcessProxy::shouldSendPendingMessage(WebKit::AuxiliaryProcessProxy::PendingMessage const&) [inlined] IPC::Encoder::messageName() const at Encoder.h:50:46 46 Encoder(MessageName, uint64_t destinationID); 47 ~Encoder(); 48 49 ReceiverName messageReceiverName() const { return receiverName(m_messageName); } -> 50 MessageName messageName() const { return m_messageName; } 51 uint64_t destinationID() const { return m_destinationID; } 52 53 bool isSyncMessage() const { return messageIsSync(messageName()); } 54 0x0000000190ba03bc: stp x29, x30, [sp, #0x40] 0x0000000190ba03c0: add x29, sp, #0x40 ; =0x40 0x0000000190ba03c4: sub sp, sp, #0x240 ; =0x240 0x0000000190ba03c8: ldr x8, [x1] -> 0x0000000190ba03cc: ldrh w22, [x8] 0x0000000190ba03d0: cmp w22, #0x65a ; =0x65a 0x0000000190ba03d4: b.ne 0x4395d0 ; <+552> at WebProcessProxy.cpp:396:40 0x0000000190ba03d8: mov x19, x0 0x0000000190ba03dc: ldr x0, [x8, #0x210] [ 0] 0x0000000190ba03cc WebKit`WebKit::WebProcessProxy::shouldSendPendingMessage(WebKit::AuxiliaryProcessProxy::PendingMessage const&) + 36 at WebProcessProxy.cpp:396 392 #endif 393 394 bool WebProcessProxy::shouldSendPendingMessage(const PendingMessage& message) 395 { -> 396 if (message.encoder->messageName() == IPC::MessageName::WebPage_LoadRequestWaitingForProcessLaunch) { 397 auto buffer = message.encoder->buffer(); 398 auto bufferSize = message.encoder->bufferSize(); 399 auto decoder = IPC::Decoder::create(buffer, bufferSize, nullptr, { }); 400 ASSERT(decoder); [ 1] 0x0000000190abd88b WebKit`WebKit::AuxiliaryProcessProxy::didFinishLaunching(WebKit::ProcessLauncher*, IPC::Connection::Identifier) + 351 at AuxiliaryProcessProxy.cpp:242:14 238 connectionWillOpen(*m_connection); 239 m_connection->open(); 240 241 for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) { -> 242 if (!shouldSendPendingMessage(pendingMessage)) 243 continue; 244 auto encoder = WTFMove(pendingMessage.encoder); 245 auto sendOptions = pendingMessage.sendOptions; 246 if (pendingMessage.asyncReplyInfo) [ 2] 0x0000000190abd88b WebKit`WebKit::AuxiliaryProcessProxy::didFinishLaunching(WebKit::ProcessLauncher*, IPC::Connection::Identifier) + 351 at AuxiliaryProcessProxy.cpp:242:14 238 connectionWillOpen(*m_connection); 239 m_connection->open(); 240 241 for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) { -> 242 if (!shouldSendPendingMessage(pendingMessage)) 243 continue; 244 auto encoder = WTFMove(pendingMessage.encoder); 245 auto sendOptions = pendingMessage.sendOptions; 246 if (pendingMessage.asyncReplyInfo) [ 3] 0x0000000190ba3163 WebKit`WebKit::WebProcessProxy::didFinishLaunching(WebKit::ProcessLauncher*, IPC::Connection::Identifier) + 123 at WebProcessProxy.cpp:1010:28 1006 { 1007 RELEASE_ASSERT(isMainThreadOrCheckDisabled()); 1008 1009 auto protectedThis = makeRef(*this); -> 1010 AuxiliaryProcessProxy::didFinishLaunching(launcher, connectionIdentifier); 1011 1012 if (!IPC::Connection::identifierIsValid(connectionIdentifier)) { 1013 RELEASE_LOG_IF(isReleaseLoggingAllowed(), Process, "%p - WebProcessProxy didFinishLaunching - invalid connection identifier (web process failed to launch)", this); 1014 processDidTerminateOrFailedToLaunch(ProcessTerminationReason::Crash); [ 4] 0x0000000190801643 WebKit`WebKit::ProcessLauncher::didFinishLaunchingProcess(int, IPC::Connection::Identifier) + 163 at ProcessLauncher.cpp:59:15 55 #endif 56 return; 57 } 58 -> 59 m_client->didFinishLaunching(this, identifier); 60 } 61 62 void ProcessLauncher::invalidate() 63 { [ 5] 0x0000000190b1f51b WebKit`invocation function for block in WebKit::ProcessLauncher::launchProcess() + 131 at ProcessLauncherMac.mm:311:13 307 308 // The process has finished launching, grab the pid from the connection. 309 pid_t processIdentifier = xpc_connection_get_pid(m_xpcConnection.get()); 310 -> 311 didFinishLaunchingProcess(processIdentifier, IPC::Connection::Identifier(listeningPort, m_xpcConnection)); 312 m_xpcConnection = nullptr; 313 } 314 315 deref(); ```
<rdar://75329251>
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].