Fix MessageReader to pass errors to the channel

Fix MessageReader to pass errors to the channel

Previously MessageReader was stopping reading after the first error,
but wasn't notifying the client about the problem. This results in some
errors (e.g. from SSL layer) being ignores while they should terminate
connection.

BUG=487451
Committed: https://crrev.com/a04c0468580e15cac8df104e63211e63c8589dda
Cr-Commit-Position: refs/heads/master@{#329780}

[Sergey Ulanov, 2015-05-13 17:58:46]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2015-05-13 17:58:53]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-05-13 17:59:06]

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

[Jamie, 2015-05-13 18:25:17]

jamiewalch@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2015-05-13 18:25:18]

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
File remoting/protocol/channel_multiplexer.cc (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.cc:343: // callbacks is enough.
But you're only resetting one of the callbacks in this case. Is that what you
want? It seems that if the right thing to do would be to invoke (and reset) both
callbacks, then the next-best thing (given that they are expected to do the same
thing) would be to invoke only one but reset them both.

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
File remoting/protocol/channel_multiplexer.h (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.h:48: void OnBaseChannelError(int error);
Why not OnReadWriteFailed for consistency with the other rename you've done?

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/messa...
File remoting/protocol/message_reader.h (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/messa...
remoting/protocol/message_reader.h:53: void HandleReadResult(int result, bool*
read_again);
I think |read_again| is a misleading name for this. It could be interpreted
(correctly, I think) that the read succeeded and it's safe to read something
else, or it could be interpreted that the read failed and should be retried. I
suggest |read_succeeded| might be a better name.

[Sergey Ulanov, 2015-05-14 00:15:21]

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
File remoting/protocol/channel_multiplexer.cc (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.cc:343: // callbacks is enough.
On 2015/05/13 18:25:17, Jamie wrote:
> But you're only resetting one of the callbacks in this case. Is that what you
> want? It seems that if the right thing to do would be to invoke (and reset)
both
> callbacks, then the next-best thing (given that they are expected to do the
same
> thing) would be to invoke only one but reset them both.

It's better to leave write_callback_ not null in that case because Write() call
is still pending.

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
File remoting/protocol/channel_multiplexer.h (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/chann...
remoting/protocol/channel_multiplexer.h:48: void OnBaseChannelError(int error);
On 2015/05/13 18:25:17, Jamie wrote:
> Why not OnReadWriteFailed for consistency with the other rename you've done?

In multiplexer OnReadWriteFailed() doesn't make it clear where the error has
occurred.

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/messa...
File remoting/protocol/message_reader.h (right):

https://codereview.chromium.org/1143443003/diff/30001/remoting/protocol/messa...
remoting/protocol/message_reader.h:53: void HandleReadResult(int result, bool*
read_again);
On 2015/05/13 18:25:17, Jamie wrote:
> I think |read_again| is a misleading name for this. It could be interpreted
> (correctly, I think) that the read succeeded and it's safe to read something
> else, or it could be interpreted that the read failed and should be retried. I
> suggest |read_succeeded| might be a better name.

Done. Note that I also changed the meaning of this parameter - it's set to true
now when result == net::ERR_IO_PENDING, i.e. ERR_IO_PENDING is not a failure. It
doesn't matter because DoRead() checks for read_pending_ anyway.

[Jamie, 2015-05-14 00:31:32]

lgtm

[Sergey Ulanov, 2015-05-14 00:46:17]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-05-14 00:47:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143443003/50001

[commit-bot: I haz the power, 2015-05-14 01:48:47]

Committed patchset #2 (id:50001)

[commit-bot: I haz the power, 2015-05-14 01:49:48]

Patchset 2 (id:??) landed as
https://crrev.com/a04c0468580e15cac8df104e63211e63c8589dda
Cr-Commit-Position: refs/heads/master@{#329780}