IPC: Remove unnecessary conversions of BrokerableAttachment.

IPC: Remove unnecessary conversions of BrokerableAttachment.

BrokerableAttachments are typically stored and passed around in a scoped_refptr.
There were several locations where they were being unnecessarily converted to a
raw pointer. This was probably responsible for a non-deterministic crash on GPU
bots (https://code.google.com/p/chromium/issues/detail?id=535711#c28), although
I haven't been able to reproduce the problem locally.

BUG=535711
Committed: https://crrev.com/a03dde6fdcd0308e897995bf2c6d00615bcda3ef
Cr-Commit-Position: refs/heads/master@{#356968}

[erikchen, 2015-10-29 00:54:21]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 00:55:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414503009/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414503009/1

[erikchen, 2015-10-29 01:06:13]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 01:07:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414503009/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414503009/20001

[commit-bot: I haz the power, 2015-10-29 01:55:28]

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

[commit-bot: I haz the power, 2015-10-29 01:55:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-10-29 03:46:39]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 03:46:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414503009/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414503009/20001

[commit-bot: I haz the power, 2015-10-29 04:22:11]

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

[commit-bot: I haz the power, 2015-10-29 04:22:11]

Dry run: This issue passed the CQ dry run.

[erikchen, 2015-10-29 17:50:48]

Description was changed from

==========
IPC: Remove unnecessary conversions of BrokerableAttachment.

BrokerableAttachments are typically stored and passed around in a scoped_refptr.
There were several locations where they were being unnecessarily converted to a
raw pointer. This was probably responsible for a non-deterministic crash on GPU
bots (https://code.google.com/p/chromium/issues/detail?id=535711#c28), although
I haven't been able to reproduce the problem locally.

BUG=535711
==========

to

==========
IPC: Remove unnecessary conversions of BrokerableAttachment.

BrokerableAttachments are typically stored and passed around in a scoped_refptr.
There were several locations where they were being unnecessarily converted to a
raw pointer. This was probably responsible for a non-deterministic crash on GPU
bots (https://code.google.com/p/chromium/issues/detail?id=535711#c28), although
I haven't been able to reproduce the problem locally.

BUG=535711
==========

[erikchen, 2015-10-29 18:40:54]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2015-10-29 18:40:54]

tsepez: Please review.

I was not able to pinpoint an exact error. GetBrokerableAttachments() seemed the
most suspicious, followed by ReplacePlaceholderWithAttachment().

[Tom Sepez, 2015-10-29 19:12:51]

Apart from the places I've marked, I think you can flip the ohters back to raw
ptrs.  Passing a const& doesn't change ref counts, so it can't affect object
lifetimes.

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_channel_reader....
ipc/ipc_channel_reader.cc:253:
std::vector<scoped_refptr<IPC::BrokerableAttachment>>
Note: this is the first place you'll actually do any refcount incrementing.

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:150: std::vector<scoped_refptr<IPC::BrokerableAttachment>>
attachments(
and here too

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
File ipc/ipc_message_attachment_set.cc (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
ipc/ipc_message_attachment_set.cc:184: scoped_refptr<BrokerableAttachment>
attachment) {
I think this can still be const&

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
File ipc/ipc_message_attachment_set.h (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
ipc/ipc_message_attachment_set.h:96: scoped_refptr<BrokerableAttachment>
attachment);
and here.

[erikchen, 2015-10-29 19:53:47]

tsepez: PTAL.

I'm aware that passing a const scoped_refptr& does not change lifetime semantics
from passing a raw pointer. My point is that there's no reason to unwrap the
scoped_refptr to expose a raw pointer, which I claim can be more easily abused.

[erikchen, 2015-10-29 19:53:54]

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_channel_reader....
ipc/ipc_channel_reader.cc:253:
std::vector<scoped_refptr<IPC::BrokerableAttachment>>
On 2015/10/29 19:12:51, Tom Sepez wrote:
> Note: this is the first place you'll actually do any refcount incrementing.

Yes.

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
File ipc/ipc_message_attachment_set.cc (right):

https://codereview.chromium.org/1414503009/diff/60001/ipc/ipc_message_attachm...
ipc/ipc_message_attachment_set.cc:184: scoped_refptr<BrokerableAttachment>
attachment) {
On 2015/10/29 19:12:51, Tom Sepez wrote:
> I think this can still be const&

Passing by const& will produce the wrong behavior for the line:
*it = attachment;

If |attachment| is the same object as *it (self assignment).

This is currently not possible, as we should always be replacing a placeholder
attachment with a non-placeholder attachment, but it is not enforced. I switched
the parameter back to const&, and then added an assertion which implicitly
enforced *it != attachment.

[Tom Sepez, 2015-10-29 21:26:19]

lgtm

[erikchen, 2015-10-29 21:27:15]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-10-29 21:27:53]

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

[commit-bot: I haz the power, 2015-10-29 22:37:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-29 22:37:53]

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