Resize IPC input buffer to fit the next message.

Resize IPC input buffer to fit the next message.

Sometimes we can get IPC message size from its header. In those cases
we resize IPC::ChannelReader' overflow buffer to fit the entire message
to avoid growing / reallocating it as we receive message's data.

BUG=529940
Committed: https://crrev.com/6f3790a15d0a5463e18dd7220e4f2028a6318389
Cr-Commit-Position: refs/heads/master@{#351586}

[Dmitry Skiba, 2015-09-21 22:13:40]

dskiba@google.com changed reviewers:
+ jam@chromium.org, mariakhomenko@chromium.org

[Maria, 2015-09-21 23:24:20]

lgtm

[jam, 2015-09-22 15:45:59]

there are test failures now?

[DmitrySkiba, 2015-09-22 15:54:54]

On 2015/09/22 15:45:59, jam wrote:
> there are test failures now?

Yup, so I'm fixing them and also writing new tests.

[Dmitry Skiba, 2015-09-24 20:15:49]

Guys, I'm done with writing tests - please review.

[Maria, 2015-09-25 22:23:01]

Looks like some bots are still failing the tests.

Also added a few style comments.

https://codereview.chromium.org/1345353004/diff/140001/base/pickle_unittest.cc
File base/pickle_unittest.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/base/pickle_unittest.c...
base/pickle_unittest.cc:344: #pragma warning(disable: 4146)
Looks like you might need to do something similar for your overflow test since
the win bot is currently failing

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:154: bool ChannelReader::CheckMessageSize(size_t size)
{
order of declarations in cc should match order of definition in .h

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:161: bool CheckMessageSize(size_t size);
Comment?

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
File ipc/ipc_channel_reader_unittest.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader_unittest.cc:146: #if !USE_ATTACHMENT_BROKER
should we just do #else here? instead of another if?

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_message_unitte...
File ipc/ipc_message_unittest.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_message_unitte...
ipc/ipc_message_unittest.cc:260: }  // namespace
if we are putting this into an anonymous namespace should we put the tests below
together with it?

[Dmitry Skiba, 2015-09-28 17:37:18]

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:154: bool ChannelReader::CheckMessageSize(size_t size)
{
On 2015/09/25 22:23:00, Maria wrote:
> order of declarations in cc should match order of definition in .h

Done.

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:161: bool CheckMessageSize(size_t size);
On 2015/09/25 22:23:00, Maria wrote:
> Comment?

Done.

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
File ipc/ipc_channel_reader_unittest.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader_unittest.cc:146: #if !USE_ATTACHMENT_BROKER
On 2015/09/25 22:23:00, Maria wrote:
> should we just do #else here? instead of another if?

The effect of #else will be the same, but it's just a matter of taste - I don't
like the implication (in my view) that #else branch is somehow related to #if
branch.

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_message_unitte...
File ipc/ipc_message_unittest.cc (right):

https://codereview.chromium.org/1345353004/diff/140001/ipc/ipc_message_unitte...
ipc/ipc_message_unittest.cc:260: }  // namespace
On 2015/09/25 22:23:00, Maria wrote:
> if we are putting this into an anonymous namespace should we put the tests
below
> together with it?

I think the convention is that helpers are in unnamed namespace, while all tests
are in named namespace (the same one where the class being tested resides).
Previously that was not the case in this file, and that caused issues with
declaring FindNext tests as IPC::Message friends, so I moved all tests into IPC
namespace, and everything else in unnamed namespaces.

[Maria, 2015-09-28 18:17:52]

lgtm

[Dmitry Skiba, 2015-09-29 18:34:53]

jam@, are you OK with this change? Waiting for your review...

[jam, 2015-09-30 14:18:16]

lgtm

sorry for the delay, I missed the updates (ping me over IM in the future if I
take more than a few hours)

https://codereview.chromium.org/1345353004/diff/160001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1345353004/diff/160001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:161: // Checks that |size| is a valid message size. Has
side effects if it's not.
nit: why does it need to mention an internal impl' detail of resetting the
buffer?

[Dmitry Skiba, 2015-09-30 15:32:35]

https://codereview.chromium.org/1345353004/diff/160001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1345353004/diff/160001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:161: // Checks that |size| is a valid message size. Has
side effects if it's not.
On 2015/09/30 14:18:16, jam wrote:
> nit: why does it need to mention an internal impl' detail of resetting the
> buffer?

Well, the function is named 'Check', so personally I wouldn't expect it to have
side effects (although lack of const-ness gives a hint). And I couldn't come up
with a better verb. Naming is hard...

[Dmitry Skiba, 2015-09-30 15:54:50]

The CQ bit was checked by dskiba@google.com

[commit-bot: I haz the power, 2015-09-30 15:55:31]

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

[commit-bot: I haz the power, 2015-09-30 17:24:40]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-30 17:25:20]

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