Update ICE protocol to handle closed channel

Update ICE protocol to handle closed channel

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167
Review-Url: https://codereview.chromium.org/2757723002
Cr-Commit-Position: refs/heads/master@{#457842}
Committed: https://chromium.googlesource.com/chromium/src/+/5b4667bc57d3ce2230e556ba38dcdc9342bdf819

[Sergey Ulanov, 2017-03-17 00:04:16]

Description was changed from

==========
Remove redundant DCHECK in message_reader.cc

An ICE/PseudoTcp channel may be closed by the peer. Read() returns 0 in
that case. MessageReader wasn't expecting 0.

BUG=700167
==========

to

==========
Update ICE protocol to handle channel closure cleanly

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167
==========

[Sergey Ulanov, 2017-03-17 00:04:53]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2017-03-17 00:05:04]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2017-03-17 00:07:20]

Description was changed from

==========
Update ICE protocol to handle channel closure cleanly

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167
==========

to

==========
Update ICE protocol to handle closed channel

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167
==========

[Sergey Ulanov, 2017-03-17 00:07:32]

sergeyu@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-03-17 15:13:28]

lgtm

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_tester.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_tester.cc:304: MessagePipe* client_pipe,
The order of host/client pipe args was changed in the CC file but not the
header.  Can you fix up the header (or the CC if the swap was not intentional)

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_unittest.cc:17: #include
"base/threading/thread_task_runner_handle.h"
I don't see any usage of base::ThreadTaskRunnerHandle, is this header needed?

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/strea...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/strea...
remoting/protocol/stream_message_pipe_adapter.cc:65: } else if
(!error_callback_.is_null()) {
nit:
  replace 
if (!error_callback_.is_null()) 
  with
if (error_callback_)

[Sergey Ulanov, 2017-03-17 18:18:22]

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_tester.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_tester.cc:304: MessagePipe* client_pipe,
On 2017/03/17 15:13:28, joedow wrote:
> The order of host/client pipe args was changed in the CC file but not the
> header.  Can you fix up the header (or the CC if the swap was not intentional)

Done. I swapped it here to match the order in the test, where this constructor
is used. Technically this order doesn't really matter (it just determines the
order in which test messages are sent, but both pipes are bi-directional).

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_unittest.cc:17: #include
"base/threading/thread_task_runner_handle.h"
On 2017/03/17 15:13:28, joedow wrote:
> I don't see any usage of base::ThreadTaskRunnerHandle, is this header needed?

Done.

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/strea...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/2757723002/diff/40001/remoting/protocol/strea...
remoting/protocol/stream_message_pipe_adapter.cc:65: } else if
(!error_callback_.is_null()) {
On 2017/03/17 15:13:28, joedow wrote:
> nit:
>   replace 
> if (!error_callback_.is_null()) 
>   with
> if (error_callback_)

Done.

[Sergey Ulanov, 2017-03-17 18:18:40]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2017-03-17 18:18:40]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2757723002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2017-03-17 18:19:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 19:12:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489774720423160,
"parent_rev": "af737fca2a12ce091d13c99cfec05d159af0748c", "commit_rev":
"5b4667bc57d3ce2230e556ba38dcdc9342bdf819"}

[commit-bot: I haz the power, 2017-03-17 19:13:40]

Description was changed from

==========
Update ICE protocol to handle closed channel

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167
==========

to

==========
Update ICE protocol to handle closed channel

An ICE/PseudoTcp channel may be closed by the peer. Read() may returns 0
in that case. MessageReader wasn't expecting 0 and was DCHECK'ing in that
case. Also update StreamMessagePipeAdapter to handle this case as closed
MessagePipe.

BUG=700167

Review-Url: https://codereview.chromium.org/2757723002
Cr-Commit-Position: refs/heads/master@{#457842}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b4667bc57d3ce2230e556ba38dc...
==========

[commit-bot: I haz the power, 2017-03-17 19:13:41]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5b4667bc57d3ce2230e556ba38dc...