Fixing a Dr. Memory hang in the remoting_unittests

Fixing a Dr. Memory hang in the remoting_unittests

There were two problems that needed to be addressed.  One was a failure that
occurred if a stream writer wrote the message type and payload in separate
write actions (timing problem) the second was a test bug in the
MultipleMessages test which did not check for success/failure for each loop
iteration.

The first problem was a timing issue where the reader might fail if the message
payload was written separately from the type and was not written quickly.  This
was not repro'd on normal builds but we see it on the Dr. Memory bot since
operations are slowed down as a result of the build flags used for the tool. I
fixed this problem by reading in the type and payload separately.

The second problem was in the unit test itself.  If an error occurred in the
Multiple messages test, the loop would write to the end of the pipe and wait
(forever) for an answer.  We now verify the message was written after every
iteration.

BUG=603357
Committed: https://crrev.com/35b79b13f17c4429107bef826c8b773f0c9b2336
Cr-Commit-Position: refs/heads/master@{#388307}

[joedow, 2016-04-18 18:30:32]

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

[joedow, 2016-04-18 18:30:33]

This change updates some behavior and fixes a unit test problem that were
causing issues on the Dr. Memory bot.

Once this change lands and runs successfully for awhile, I am going to send a
second CL out which removes the Dr. Memory exception for the MultipleMessages
test.

PTAL!

Thanks,
Joe

[joedow, 2016-04-18 18:30:34]

This change updates some behavior and fixes a unit test problem that were
causing issues on the Dr. Memory bot.

Once this change lands and runs successfully for awhile, I am going to send a
second CL out which removes the Dr. Memory exception for the MultipleMessages
test.

PTAL!

Thanks,
Joe

[Sergey Ulanov, 2016-04-18 18:57:32]

https://codereview.chromium.org/1900733002/diff/40001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_impl.cc
(right):

https://codereview.chromium.org/1900733002/diff/40001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:102:
read_stream_.ReadAtCurrentPos(&message_data[1], remaining_bytes);
I don't understand why you need to call ReadAtCurrentPos() twice.
ReadAtCurrentPos() is supposed to read as many bites as requested, unless error
occurs. It shouldn't matter if the other end writes data in chunks or not.
Otherwise you would need to handle partial reads for payload as well (in case
it's sent with 2 write() calls).

The posix version of ReadAtCurrenPos() handles partial reads explicitly by
retrying read(), but the windows version doesn't:
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_wi...
It's not clear to me what's the expected behavior for ReadFile() when reading
from pipes, but it's likely that it can performs partial reads there. So it
looks like the problem here is that ReadAtCurrenPos() behavior is not consistent
across the two implementations (at least when used for pipes). The comments for
ReadAtCurrenPos() say that "it is not intended for stream oriented files". I
think we need to either use ReadAtCurrentPosNoBestEffort() and retry after
partial reads or to fix the windows implementation of ReadAtCurrenPos() to make
it consistent with POSIX.

[joedow, 2016-04-19 17:28:10]

Great feedback, updated Cl based on it.

https://codereview.chromium.org/1900733002/diff/40001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_impl.cc
(right):

https://codereview.chromium.org/1900733002/diff/40001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:102:
read_stream_.ReadAtCurrentPos(&message_data[1], remaining_bytes);
On 2016/04/18 18:57:32, Sergey Ulanov wrote:
> I don't understand why you need to call ReadAtCurrentPos() twice.
> ReadAtCurrentPos() is supposed to read as many bites as requested, unless
error
> occurs. It shouldn't matter if the other end writes data in chunks or not.
> Otherwise you would need to handle partial reads for payload as well (in case
> it's sent with 2 write() calls).
> 
> The posix version of ReadAtCurrenPos() handles partial reads explicitly by
> retrying read(), but the windows version doesn't:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_wi...
> It's not clear to me what's the expected behavior for ReadFile() when reading
> from pipes, but it's likely that it can performs partial reads there. So it
> looks like the problem here is that ReadAtCurrenPos() behavior is not
consistent
> across the two implementations (at least when used for pipes). The comments
for
> ReadAtCurrenPos() say that "it is not intended for stream oriented files". I
> think we need to either use ReadAtCurrentPosNoBestEffort() and retry after
> partial reads or to fix the windows implementation of ReadAtCurrenPos() to
make
> it consistent with POSIX.

This is a good point, my workaround isn't robust enough.  I agree that the
behavior for the Read* functions feels wrong.  I am a bit hesitant to change the
base function since it is simple enough to treat it as a non-blocking read
operation.

I'd like to fix this issue to unblock my change and the Dr. memory fix and have
filed crbug.com/604787 to track the file change.

[Sergey Ulanov, 2016-04-19 17:36:40]

lgtm when my comments are addressed

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_impl.cc
(right):

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:102:
uint32_t bytes_to_read) {
size_t

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:106:
uint32_t bytes_read = 0;
size_t

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:108: int
read_result = read_stream_.ReadAtCurrentPos(buffer + bytes_read,
Use ReadAtCurrentPosNoBestEffort() here?

[joedow, 2016-04-19 21:07:24]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-04-19 21:07:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1900733002/#ps100001
(title: "Addressing CR feedback")

[commit-bot: I haz the power, 2016-04-19 21:07:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900733002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900733002/100001

[joedow, 2016-04-19 21:07:56]

Thanks!

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_impl.cc
(right):

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:102:
uint32_t bytes_to_read) {
On 2016/04/19 17:36:39, Sergey Ulanov wrote:
> size_t

Done.

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:106:
uint32_t bytes_read = 0;
On 2016/04/19 17:36:40, Sergey Ulanov wrote:
> size_t

Done.

https://codereview.chromium.org/1900733002/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_impl.cc:108: int
read_result = read_stream_.ReadAtCurrentPos(buffer + bytes_read,
On 2016/04/19 17:36:40, Sergey Ulanov wrote:
> Use ReadAtCurrentPosNoBestEffort() here?

Done.

[commit-bot: I haz the power, 2016-04-19 21:11:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:16:16]

Description was changed from

==========
Fixing a Dr. Memory hang in the remoting_unittests

There were two problems that needed to be addressed.  One was a failure that
occurred if a stream writer wrote the message type and payload in separate
write actions (timing problem) the second was a test bug in the
MultipleMessages test which did not check for success/failure for each loop
iteration.

The first problem was a timing issue where the reader might fail if the message
payload was written separately from the type and was not written quickly.  This
was not repro'd on normal builds but we see it on the Dr. Memory bot since
operations are slowed down as a result of the build flags used for the tool. I
fixed this problem by reading in the type and payload separately.

The second problem was in the unit test itself.  If an error occurred in the
Multiple messages test, the loop would write to the end of the pipe and wait
(forever) for an answer.  We now verify the message was written after every
iteration.

BUG=603357
==========

to

==========
Fixing a Dr. Memory hang in the remoting_unittests

There were two problems that needed to be addressed.  One was a failure that
occurred if a stream writer wrote the message type and payload in separate
write actions (timing problem) the second was a test bug in the
MultipleMessages test which did not check for success/failure for each loop
iteration.

The first problem was a timing issue where the reader might fail if the message
payload was written separately from the type and was not written quickly.  This
was not repro'd on normal builds but we see it on the Dr. Memory bot since
operations are slowed down as a result of the build flags used for the tool. I
fixed this problem by reading in the type and payload separately.

The second problem was in the unit test itself.  If an error occurred in the
Multiple messages test, the loop would write to the end of the pipe and wait
(forever) for an answer.  We now verify the message was written after every
iteration.

BUG=603357

Committed: https://crrev.com/35b79b13f17c4429107bef826c8b773f0c9b2336
Cr-Commit-Position: refs/heads/master@{#388307}
==========

[commit-bot: I haz the power, 2016-04-22 19:16:17]

Patchset 6 (id:??) landed as
https://crrev.com/35b79b13f17c4429107bef826c8b773f0c9b2336
Cr-Commit-Position: refs/heads/master@{#388307}