Crash the network or desktop process when an unknown IPC message is received.

Crash the network or desktop process when an unknown IPC message is received.

When an unknown IPC message is received by the daemon, the sender process is asked to terminate itself to generate a crash dump. When such a message is received by either network or desktop process the receiver CHECK(). The crashed process will be automatically restarted by the daemon (applying exponential backoff as needed).

BUG=179215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186274

[alexeypa (please no reviews), 2013-02-28 23:50:33]

PTAL.

[Wez, 2013-02-28 23:53:13]

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:153: DeleteAllDesktopSessions();
nit: We should really tear-down the IPC channel to the process and, after some
timeout, forcibly terminate the process.

[alexeypa (please no reviews), 2013-03-01 00:09:49]

On 2013/02/28 23:53:13, Wez wrote:
>
https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
> File remoting/host/daemon_process.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
> remoting/host/daemon_process.cc:153: DeleteAllDesktopSessions();
> nit: We should really tear-down the IPC channel to the process and, after some
> timeout, forcibly terminate the process.

Lambros asked me to find a reviewer more familiar with this code. Wez, since you
are here already, why don't you take a closer look? :-)

[Wez, 2013-03-04 23:17:09]

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:74: return handled;
Do you know what IpcChannel itself does with the |handled| result?  e.g. does it
call OnChannelError(), and/or tear the channel down?

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:151: location.function_name(),
location.file_name(), location.line_number()));
nit: You're not closing the IPC channel here, so DeleteAllDesktopSessions may
result in IPCs being dispatched to the network process, which you no longer
trust, which doesn't seem right.

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/desktop_...
File remoting/host/desktop_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/desktop_...
remoting/host/desktop_process.cc:65: CHECK(handled) << "An unexpected IPC
message received: type="
nit: "Received unexpected IPC type:"?

[alexeypa (please no reviews), 2013-03-05 00:01:44]

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:74: return handled;
On 2013/03/04 23:17:09, Wez wrote:
> Do you know what IpcChannel itself does with the |handled| result?  e.g. does
it
> call OnChannelError(), and/or tear the channel down?

It ignores the returned value.

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:151: location.function_name(),
location.file_name(), location.line_number()));
On 2013/03/04 23:17:09, Wez wrote:
> nit: You're not closing the IPC channel here, so DeleteAllDesktopSessions may
> result in IPCs being dispatched to the network process, which you no longer
> trust, which doesn't seem right.

Agree. The current implementation does not enforce anything. It is more of a
diagnostic tool.

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:153: DeleteAllDesktopSessions();
On 2013/02/28 23:53:14, Wez wrote:
> nit: We should really tear-down the IPC channel to the process and, after some
> timeout, forcibly terminate the process.

Agree, but let's do it as a separate CL. I imagine it should be a part of the
worker process launcher.

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/desktop_...
File remoting/host/desktop_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/desktop_...
remoting/host/desktop_process.cc:65: CHECK(handled) << "An unexpected IPC
message received: type="
On 2013/03/04 23:17:09, Wez wrote:
> nit: "Received unexpected IPC type:"?

Done.

[Wez, 2013-03-05 02:12:30]

LGTM for the CHECKs and RestartDesktopProcess() call but not sure of the value
of the network process crash?

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:153: DeleteAllDesktopSessions();
On 2013/03/05 00:01:44, alexeypa wrote:
> On 2013/02/28 23:53:14, Wez wrote:
> > nit: We should really tear-down the IPC channel to the process and, after
some
> > timeout, forcibly terminate the process.
> 
> Agree, but let's do it as a separate CL. I imagine it should be a part of the
> worker process launcher.

SGTM.

https://chromiumcodereview.appspot.com/12390027/diff/4002/remoting/host/daemo...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/4002/remoting/host/daemo...
remoting/host/daemon_process.cc:71: CrashNetworkProcess(FROM_HERE);
Why do we want to crash the network process?  I don't think we capture memory
state in crash logs, so we'll just get a stack containing the IPC processing for
the crash message?

[alexeypa (please no reviews), 2013-03-05 18:09:15]

https://chromiumcodereview.appspot.com/12390027/diff/4002/remoting/host/daemo...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12390027/diff/4002/remoting/host/daemo...
remoting/host/daemon_process.cc:71: CrashNetworkProcess(FROM_HERE);
On 2013/03/05 02:12:31, Wez wrote:
> Why do we want to crash the network process?  I don't think we capture memory
> state in crash logs, so we'll just get a stack containing the IPC processing
for
> the crash message?

The network process dump will capture the location when CrashNetworkProcess()
was called in the daemon's code. Not much, but still useful.
CrashNetworkProcess() is called to handle other protocol errors as well.

[commit-bot: I haz the power, 2013-03-05 19:58:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12390027/12001

[commit-bot: I haz the power, 2013-03-05 20:07:18]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-05 20:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12390027/22001

[commit-bot: I haz the power, 2013-03-05 20:54:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[alexeypa (please no reviews), 2013-03-05 22:47:30]

Committed patchset #4 manually as r186274 (presubmit successful).