Disconnect native messaging port on read pipe EOF.

Disconnect native messaging port on read pipe EOF.

Previously native messaging ports were kept open even after native messaging
host has closed stdout pipe which is used to send messages to the connected
webapp. That behavior makes it hard to detect the case when the host exists
but fails to start. Now chrome will close the native messaging port as soon
as it reads EOF from the pipe.

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

[Matt Perry, 2013-09-24 20:27:36]

lgtm

[commit-bot: I haz the power, 2013-09-24 23:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/24409004/38001

[commit-bot: I haz the power, 2013-09-25 01:32:12]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-25 01:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/24409004/38001

[Sergey Ulanov, 2013-09-25 20:02:28]

+akalin - Fred, please approve net/base change.

[akalin, 2013-09-25 21:48:03]

lgtm for net/base, but i'd like wtc@ to double-check since I'm not that familiar
with windows errors

[wtc, 2013-09-25 23:03:12]

Patch set 5 LGTM.

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:250: }
else if (result == 0 || result == net::ERR_CONNECTION_RESET) {

On Unix, EPIPE is an error code for write/send. Here we are handling read
result. Can we get net::ERR_CONNECTION_RESET from read on Windows?

https://codereview.chromium.org/24409004/diff/82001/net/base/net_errors_win.cc
File net/base/net_errors_win.cc (right):

https://codereview.chromium.org/24409004/diff/82001/net/base/net_errors_win.c...
net/base/net_errors_win.cc:114: return ERR_CONNECTION_RESET;

On Unix, we map EPIPE to ERR_CONNECTION_RESET. The error message for EPIPE is
"Broken pipe". So it seems that this error code mapping is correct, even though
I am also not familiar with the Windows ERROR_BROKEN_PIPE error code :-)

[akalin, 2013-09-25 23:09:32]

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:250: }
else if (result == 0 || result == net::ERR_CONNECTION_RESET) {
On 2013/09/25 23:03:12, wtc wrote:
> 
> On Unix, EPIPE is an error code for write/send. Here we are handling read
> result. Can we get net::ERR_CONNECTION_RESET from read on Windows?

I'm guessing that we're getting ERROR_BROKEN_PIPE on read on Windows, and so the
new mapping will then generate this case.

[Sergey Ulanov, 2013-09-25 23:30:50]

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:250: }
else if (result == 0 || result == net::ERR_CONNECTION_RESET) {
On 2013/09/25 23:03:12, wtc wrote:
> 
> On Unix, EPIPE is an error code for write/send. Here we are handling read
> result. Can we get net::ERR_CONNECTION_RESET from read on Windows?

On windows we always get ERROR_BROKEN_PIPE when reading from a broken pipe. On
posix read() returns 0 in that case.

[commit-bot: I haz the power, 2013-09-26 04:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/24409004/82001

[commit-bot: I haz the power, 2013-09-26 09:31:26]

Change committed as 225418

[wtc, 2013-09-26 17:14:05]

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/24409004/diff/82001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:250: }
else if (result == 0 || result == net::ERR_CONNECTION_RESET) {
On 2013/09/25 23:30:51, Sergey Ulanov wrote:
>
> On windows we always get ERROR_BROKEN_PIPE when reading from a broken pipe. On
> posix read() returns 0 in that case.

Thanks. It would be nice to add a comment about this.

I found this behavior documented in the MSDN page for ReadFile():

==========
Pipes

If an anonymous pipe is being used and the write handle has been closed, when
ReadFile attempts to read using the pipe's corresponding read handle, the
function returns FALSE and GetLastError returns ERROR_BROKEN_PIPE.

...

If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a
pipe, the other end of the pipe called the WriteFile function with
nNumberOfBytesToWrite set to zero.
==========

Are you using an anonymous pipe and reading from it with ReadFile()?

If we disallow writing 0 bytes to the pipe, then we can change the
ERROR_BROKEN_PIPE error to a read byte count of 0 with no risk of confusion.
Would you be willing to do that?

Alternatively, it may be a good idea to perform the result ==
net::ERR_CONNECTION_RESET check on Windows only, because on Unix
the EPIPE or ECONNRESET error is a sign of an abrupt (not graceful) connection
closure.

[Sergey Ulanov, 2013-09-26 17:37:40]

On 2013/09/26 17:14:05, wtc wrote:
> 
> Are you using an anonymous pipe and reading from it with ReadFile()?

No. We are using named pipes for native messaging. Anonymous pipes are rather
limited on windows (e.g. they don't support overlapped IO).


> 
> If we disallow writing 0 bytes to the pipe, then we can change the
> ERROR_BROKEN_PIPE error to a read byte count of 0 with no risk of confusion.
> Would you be willing to do that?

This doesn't seem like a right thing to do. There may be other cases when
ERROR_BROKEN_PIPE is returned, in which returning 0 would not be correct.

> 
> Alternatively, it may be a good idea to perform the result ==
> net::ERR_CONNECTION_RESET check on Windows only, because on Unix
> the EPIPE or ECONNRESET error is a sign of an abrupt (not graceful) connection
> closure.

read() always returns 0 for not connected pipes. write() may return EPIPE, but
it depends on state of SIGPIPE handler an SO_NOSIGPIPE option. It doesn't matter
how the other end of the pipe was closed.

[wtc, 2013-09-27 19:01:48]

On 2013/09/26 17:37:40, Sergey Ulanov wrote:
> On 2013/09/26 17:14:05, wtc wrote:
> > 
> > If we disallow writing 0 bytes to the pipe, then we can change the
> > ERROR_BROKEN_PIPE error to a read byte count of 0 with no risk of confusion.
> > Would you be willing to do that?
> 
> This doesn't seem like a right thing to do. There may be other cases when
> ERROR_BROKEN_PIPE is returned, in which returning 0 would not be correct.

I found this example code on MSDN:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365588%28v=vs.85%29...

It tells us two things.

1. It suggests the documentation of ERROR_BROKEN_PIPE in the MSDN page for
ReadFile()
applies not only to an anonymous pipe but also to a named pipe.

2. It suggests the ERROR_BROKEN_PIPE error is only used to indicate the peer
disconnected.

So it should be safe to hide this platform difference and use a read byte count
of 0
to report this condition. In any case, I leave the decision to you.

> > Alternatively, it may be a good idea to perform the result ==
> > net::ERR_CONNECTION_RESET check on Windows only, because on Unix
> > the EPIPE or ECONNRESET error is a sign of an abrupt (not graceful)
connection
> > closure.
> 
> read() always returns 0 for not connected pipes. write() may return EPIPE, but
> it depends on state of SIGPIPE handler an SO_NOSIGPIPE option. It doesn't
matter
> how the other end of the pipe was closed.

Thanks. So I think you agree that on Unix, read() won't fail with EPIPE or
ECONNRESET
under the same condition, so the result == net::ERR_CONNECTION_RESET check can
be
made Windows only. The #if defined(OS_WIN) can make the code ugly, so I think
the
comment you just added in https://codereview.chromium.org/24806002/ is fine.