Crash under WebProcessProxy::shouldSendPendingMessage()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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();
```

[Chris Dumez]

<rdar://75329251>

[Chris Dumez]

Created attachment 425630 [details]
Patch

[Chris Dumez]

Comment on attachment 425630 [details]
Patch

Actually, sendMessage() relies on state() so it is still not thread-safe. I'll rework this.

[Chris Dumez]

Created attachment 425644 [details]
Patch

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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`?

[Chris Dumez]

Created attachment 425690 [details]
Patch

[Chris Dumez]

(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.

[EWS]

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].