[GCM] Fix crash when size packet splits two socket reads

[GCM] Fix crash when size packet splits two socket reads

ByteCount(), rather than UnreadByteCount(), was being incorrectly used to
determine how to handle a size packet than needed a second byte.
ByteCount() includes read bytes, while UnreadByteCount() is the one that does
not. UnreadByteCount is the only one that should be used here. Fixed, and
added new tests.

BUG=424532
Committed: https://crrev.com/7592d13c4d3b73bcfae418bf451981f72fa9bd77
Cr-Commit-Position: refs/heads/master@{#300525}

[Nicolas Zea, 2014-10-20 21:38:02]

Patchset #2 (id:20001) has been deleted

[Nicolas Zea, 2014-10-20 21:38:28]

zea@chromium.org changed reviewers:
+ fgorski@chromium.org

[Nicolas Zea, 2014-10-20 21:38:29]

PTAL

[fgorski, 2014-10-20 22:11:55]

lgtm with a few nits and a question.

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl.cc (right):

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:330: int prev_byte_count =
input_stream_->UnreadByteCount();
please explain why you are switching to UnreadByteCount()

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:384: if (protobuf.get()) {
nit: did you consider putting all of the failure conditions first and leave that
portion at the end?

You won't need else and it will look closer to what was there before.

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl_unittest.cc (right):

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl_unittest.cc:750:
EncodePacket(100, invalid_message);
nit: could you name that value? could be a const internal to the test. That is
to call it out, and perhaps fix it in distant future whenever 100 is a correct
message tag.

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl_unittest.cc:770: // Receive a
message where the size field spans two socket reads.
nit: pleas call out that it is byte 2 and 3 explicitly (that is what I am
reading from the test assuming that the first byte is the tag). It will make it
more obvious why you picked 2 as a split point.

[Nicolas Zea, 2014-10-21 00:02:54]

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl.cc (right):

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:330: int prev_byte_count =
input_stream_->UnreadByteCount();
On 2014/10/20 22:11:54, fgorski wrote:
> please explain why you are switching to UnreadByteCount()

I've updated the commit comment to explain why, but in general it was just a
mistake to use ByteCount in the first place (UnreadByteCount is whats used
everywhere else in this file and elsewhere).

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:384: if (protobuf.get()) {
On 2014/10/20 22:11:54, fgorski wrote:
> nit: did you consider putting all of the failure conditions first and leave
that
> portion at the end?
> 
> You won't need else and it will look closer to what was there before.

Done.

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl_unittest.cc (right):

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl_unittest.cc:750:
EncodePacket(100, invalid_message);
On 2014/10/20 22:11:55, fgorski wrote:
> nit: could you name that value? could be a const internal to the test. That is
> to call it out, and perhaps fix it in distant future whenever 100 is a correct
> message tag. 

Done.

https://codereview.chromium.org/643133003/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl_unittest.cc:770: // Receive a
message where the size field spans two socket reads.
On 2014/10/20 22:11:55, fgorski wrote:
> nit: pleas call out that it is byte 2 and 3 explicitly (that is what I am
> reading from the test assuming that the first byte is the tag). It will make
it
> more obvious why you picked 2 as a split point.

Done.

[Nicolas Zea, 2014-10-21 00:02:58]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:04:46]

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

[commit-bot: I haz the power, 2014-10-21 02:08:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:08:49]

Exceeded time limit waiting for builds to trigger.

[Nicolas Zea, 2014-10-21 17:38:06]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-10-21 17:39:53]

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

[commit-bot: I haz the power, 2014-10-21 18:21:54]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-21 18:22:41]

Patchset 3 (id:??) landed as
https://crrev.com/7592d13c4d3b73bcfae418bf451981f72fa9bd77
Cr-Commit-Position: refs/heads/master@{#300525}