974176 - Windows IPC channel implementation can get stuck in ChannelOpening if a child process crashes to early

Status: NEW

(Core :: IPC, defect, P3)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Add a process watch to ipc_channel_win, v1

Bug 956218 turned the dom/ipc/tests/test_process_error.xul orange reliably on all Windows builds. The test basically launches a child process and then tells it to kill itself immediately. The PBackground test code that I added opens an IPC channel to the parent on startup, but then the child process dies before the child ever connects to the parent pipe. The Windows channel implementation doesn't notice that the child died and so the channel and its IPDL actors never get torn down properly. My code spins the event loop on shutdown waiting for all PBackground actors to clean themselves up, so the test causes us to hang on shutdown.

The code path that we go through for 'opens' protocols (like PBackground, PCompositor, PImageBridge) is different from the code path that we go through for PContent, so I don't think we have this same problem for normal process launch.

The posix IPC channel implementation seems to handle this case (at least, the test is green on all non-Windows builds), though I have no idea how.

The attached patch fixes the problem locally and on try. It works by adding a process handle watch to the channel implementation when the pipe is being opened by another process. This code doesn't really have an owner so I'm going to try seein gif bbondy wants to review it.

Comment 1

[Brian R. Bondy [:bbondy]]

Comment on attachment 8377916 [details] [diff] [review]
Add a process watch to ipc_channel_win, v1

Review of attachment 8377916 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me but I'd pass it by an IPC peer

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> This looks good to me but I'd pass it by an IPC peer

Thanks Brian!

Comment 3

[Benjamin Smedberg]

Comment on attachment 8377916 [details] [diff] [review]
Add a process watch to ipc_channel_win, v1

I at least need some serious comments explaining why we need kProcessExitHandlerBit and how MessagePumpForIO::WaitForWork uses it to distinguish... something.