RenderThreadImpl::Send() failure should take down the renderer process. |
|||
Issue descriptionContext: In the fast shutdown path, the browser kills its end of the IPC channel, and expects the Renderer to terminate cleanly. This means that any IPC message sent from the renderer can fail. Either: 1) Every caller of RenderThreadImpl::Send() needs to handle errors gracefully, or else 2) A failure in RenderThreadImpl::Send() should take down the process, since it should no longer have an impact on the user. Also see: https://bugs.chromium.org/p/chromium/issues/detail?id=615121 I've talked with avi@, rockot@, and piman@, all of whom think (2) is the right approach. Unfortunately, the naive CL to implement (2) has test failures: https://codereview.chromium.org/2038633002/ After some digging, it looks like there's GPU code that will set set_reply_error() on Sync IPC messages to indicate that the message did NOT accomplish the original task: https://cs-staging.chromium.org/chromium/src/content/browser/renderer_host/render_message_filter.cc?q=set_reply_error+file:cc$&sq=package:chromium&dr=C&l=744 There are other uses of set_reply_error() in GPU code, NaCl, and elsewhere: https://cs-staging.chromium.org/search/?q=set_reply_error+file:cc$&sq=package:chromium&type=cs Unfortunately, RenderThreadImpl::Send() has no way of differentiating a recoverable error (where callers actually expect to use the return value of Send()), and an irrecoverable Channel error. It's possible that we want to systematically re-examine the semantics of Sync IPC messages, but in the short term, I suggest: 1) Rewrite the interface the RenderThreadImpl to have two functions: void Send(Message* msg); bool SendSyncAllowErrorReply(SyncMessage* msg) WARN_UNUSED_RESULT; Both will exit() on channel error, but the latter allows existing consumers of set_reply_error() to still get their responses. This may require plumbing some changes up the class hierarchy, potentially all the way to IPC::Sender. I need to look at whether IPC::Sender() can be privately inherited. This will require changing the interface of SyncChannel to differentiate unrecoverable channel errors from a recoverable, expected, *error* response.
,
Jun 6 2016
,
Jun 6 2016
""" An alternative to your proposal is to make it a property the IPC::Channel, that causes it to terminate the process when the channel is lost. Any child could set that property on its master channel, but other channels would not be affected. This would be purely independent of the message reply error bit. """ This would be the easiest solution, but given that we are phasing out Chrome IPC anyways, I don't see much point in changing it. If we think this is the right solution, we should implement it in Mojo IPC. From a semantics perspective, it seems like the owner of a given IPC::Channel/Message Pipe is best situated to know whether a failure should take down the process, and it seems incorrect to plumb that logic into the ipc layer. rockot@, jam@, thoughts on whether the logic to take down the process on IPC failure should happen at the IPC layer, or somewhere higher?
,
Jun 6 2016
If not a property of the channel, maybe a SendFailureObserver, that would run on the IO thread, prior to unblocking the main thread? ChildProcessImpl could handle that and terminate.
,
Jun 9 2016
The suggestion in #4 sounds reasonable. It's also something we could extend into Mojo fairly easily, i.e. when you connect the EDK to a new process you could give it a closure to invoke if the associated OS pipe is broken.
,
Oct 17
|
|||
►
Sign in to add a comment |
|||
Comment 1 by piman@chromium.org
, Jun 3 2016