diff options
author | erikchen <erikchen@chromium.org> | 2015-10-29 15:37:04 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-10-29 22:37:43 +0000 |
commit | a03dde6fdcd0308e897995bf2c6d00615bcda3ef (patch) | |
tree | c4de89ba410cd0d39ac0da947ce31b94227288d2 | |
parent | a584b9ec2e70e2ea2a56ca9698596ad5e5afea81 (diff) | |
download | chromium_src-a03dde6fdcd0308e897995bf2c6d00615bcda3ef.zip chromium_src-a03dde6fdcd0308e897995bf2c6d00615bcda3ef.tar.gz chromium_src-a03dde6fdcd0308e897995bf2c6d00615bcda3ef.tar.bz2 |
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
Review URL: https://codereview.chromium.org/1414503009
Cr-Commit-Position: refs/heads/master@{#356968}
-rw-r--r-- | ipc/attachment_broker.h | 6 | ||||
-rw-r--r-- | ipc/attachment_broker_mac_unittest.cc | 5 | ||||
-rw-r--r-- | ipc/attachment_broker_privileged_mac.cc | 4 | ||||
-rw-r--r-- | ipc/attachment_broker_privileged_mac.h | 5 | ||||
-rw-r--r-- | ipc/attachment_broker_privileged_win.cc | 4 | ||||
-rw-r--r-- | ipc/attachment_broker_privileged_win.h | 5 | ||||
-rw-r--r-- | ipc/attachment_broker_privileged_win_unittest.cc | 5 | ||||
-rw-r--r-- | ipc/attachment_broker_unprivileged_mac.cc | 4 | ||||
-rw-r--r-- | ipc/attachment_broker_unprivileged_mac.h | 5 | ||||
-rw-r--r-- | ipc/attachment_broker_unprivileged_win.cc | 4 | ||||
-rw-r--r-- | ipc/attachment_broker_unprivileged_win.h | 5 | ||||
-rw-r--r-- | ipc/ipc_channel_posix.cc | 2 | ||||
-rw-r--r-- | ipc/ipc_channel_reader.cc | 6 | ||||
-rw-r--r-- | ipc/ipc_channel_reader_unittest.cc | 5 | ||||
-rw-r--r-- | ipc/ipc_channel_win.cc | 2 | ||||
-rw-r--r-- | ipc/ipc_message.cc | 7 | ||||
-rw-r--r-- | ipc/ipc_message_attachment_set.cc | 18 | ||||
-rw-r--r-- | ipc/ipc_message_attachment_set.h | 3 |
18 files changed, 48 insertions, 47 deletions
diff --git a/ipc/attachment_broker.h b/ipc/attachment_broker.h index 36489d4..a015587 100644 --- a/ipc/attachment_broker.h +++ b/ipc/attachment_broker.h @@ -7,6 +7,7 @@ #include "base/gtest_prod_util.h" #include "base/macros.h" +#include "base/memory/ref_counted.h" #include "base/process/process_handle.h" #include "ipc/brokerable_attachment.h" #include "ipc/ipc_export.h" @@ -65,8 +66,9 @@ class IPC_EXPORT AttachmentBroker : public Listener { // IPC::Channel to communicate with the broker process. This may be the same // IPC::Channel that is requesting the brokering of an attachment. // Returns true on success and false otherwise. - virtual bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) = 0; + virtual bool SendAttachmentToProcess( + const scoped_refptr<BrokerableAttachment>& attachment, + base::ProcessId destination_process) = 0; // Returns whether the attachment was available. If the attachment was // available, populates the output parameter |attachment|. diff --git a/ipc/attachment_broker_mac_unittest.cc b/ipc/attachment_broker_mac_unittest.cc index 4d1f5bc..e3062b5 100644 --- a/ipc/attachment_broker_mac_unittest.cc +++ b/ipc/attachment_broker_mac_unittest.cc @@ -264,8 +264,9 @@ class MockBroker : public IPC::AttachmentBrokerUnprivilegedMac { public: MockBroker() {} ~MockBroker() override {} - bool SendAttachmentToProcess(IPC::BrokerableAttachment* attachment, - base::ProcessId destination_process) override { + bool SendAttachmentToProcess( + const scoped_refptr<IPC::BrokerableAttachment>& attachment, + base::ProcessId destination_process) override { return IPC::AttachmentBrokerUnprivilegedMac::SendAttachmentToProcess( attachment, base::Process::Current().Pid()); } diff --git a/ipc/attachment_broker_privileged_mac.cc b/ipc/attachment_broker_privileged_mac.cc index 61694bf..8965eb5 100644 --- a/ipc/attachment_broker_privileged_mac.cc +++ b/ipc/attachment_broker_privileged_mac.cc @@ -60,12 +60,12 @@ AttachmentBrokerPrivilegedMac::AttachmentBrokerPrivilegedMac( AttachmentBrokerPrivilegedMac::~AttachmentBrokerPrivilegedMac() {} bool AttachmentBrokerPrivilegedMac::SendAttachmentToProcess( - BrokerableAttachment* attachment, + const scoped_refptr<IPC::BrokerableAttachment>& attachment, base::ProcessId destination_process) { switch (attachment->GetBrokerableType()) { case BrokerableAttachment::MACH_PORT: { internal::MachPortAttachmentMac* mach_port_attachment = - static_cast<internal::MachPortAttachmentMac*>(attachment); + static_cast<internal::MachPortAttachmentMac*>(attachment.get()); MachPortWireFormat wire_format = mach_port_attachment->GetWireFormat(destination_process); diff --git a/ipc/attachment_broker_privileged_mac.h b/ipc/attachment_broker_privileged_mac.h index 329a376..d20bcc5 100644 --- a/ipc/attachment_broker_privileged_mac.h +++ b/ipc/attachment_broker_privileged_mac.h @@ -55,8 +55,9 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac ~AttachmentBrokerPrivilegedMac() override; // IPC::AttachmentBroker overrides. - bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) override; + bool SendAttachmentToProcess( + const scoped_refptr<IPC::BrokerableAttachment>& attachment, + base::ProcessId destination_process) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; diff --git a/ipc/attachment_broker_privileged_win.cc b/ipc/attachment_broker_privileged_win.cc index a638752f..ec84940 100644 --- a/ipc/attachment_broker_privileged_win.cc +++ b/ipc/attachment_broker_privileged_win.cc @@ -19,12 +19,12 @@ AttachmentBrokerPrivilegedWin::AttachmentBrokerPrivilegedWin() {} AttachmentBrokerPrivilegedWin::~AttachmentBrokerPrivilegedWin() {} bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess( - BrokerableAttachment* attachment, + const scoped_refptr<IPC::BrokerableAttachment>& attachment, base::ProcessId destination_process) { switch (attachment->GetBrokerableType()) { case BrokerableAttachment::WIN_HANDLE: { const internal::HandleAttachmentWin* handle_attachment = - static_cast<const internal::HandleAttachmentWin*>(attachment); + static_cast<const internal::HandleAttachmentWin*>(attachment.get()); HandleWireFormat wire_format = handle_attachment->GetWireFormat(destination_process); HandleWireFormat new_wire_format = diff --git a/ipc/attachment_broker_privileged_win.h b/ipc/attachment_broker_privileged_win.h index f40ccf5..e12e505 100644 --- a/ipc/attachment_broker_privileged_win.h +++ b/ipc/attachment_broker_privileged_win.h @@ -20,8 +20,9 @@ class IPC_EXPORT AttachmentBrokerPrivilegedWin ~AttachmentBrokerPrivilegedWin() override; // IPC::AttachmentBroker overrides. - bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) override; + bool SendAttachmentToProcess( + const scoped_refptr<IPC::BrokerableAttachment>& attachment, + base::ProcessId destination_process) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; diff --git a/ipc/attachment_broker_privileged_win_unittest.cc b/ipc/attachment_broker_privileged_win_unittest.cc index 555262a..b0353d6 100644 --- a/ipc/attachment_broker_privileged_win_unittest.cc +++ b/ipc/attachment_broker_privileged_win_unittest.cc @@ -276,8 +276,9 @@ class MockBroker : public IPC::AttachmentBrokerUnprivilegedWin { public: MockBroker() {} ~MockBroker() override {} - bool SendAttachmentToProcess(IPC::BrokerableAttachment* attachment, - base::ProcessId destination_process) override { + bool SendAttachmentToProcess( + const scoped_refptr<IPC::BrokerableAttachment>& attachment, + base::ProcessId destination_process) override { return IPC::AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess( attachment, base::Process::Current().Pid()); } diff --git a/ipc/attachment_broker_unprivileged_mac.cc b/ipc/attachment_broker_unprivileged_mac.cc index b5a7e84..2bba01d 100644 --- a/ipc/attachment_broker_unprivileged_mac.cc +++ b/ipc/attachment_broker_unprivileged_mac.cc @@ -50,12 +50,12 @@ AttachmentBrokerUnprivilegedMac::AttachmentBrokerUnprivilegedMac() {} AttachmentBrokerUnprivilegedMac::~AttachmentBrokerUnprivilegedMac() {} bool AttachmentBrokerUnprivilegedMac::SendAttachmentToProcess( - BrokerableAttachment* attachment, + const scoped_refptr<IPC::BrokerableAttachment>& attachment, base::ProcessId destination_process) { switch (attachment->GetBrokerableType()) { case BrokerableAttachment::MACH_PORT: { internal::MachPortAttachmentMac* mach_port_attachment = - static_cast<internal::MachPortAttachmentMac*>(attachment); + static_cast<internal::MachPortAttachmentMac*>(attachment.get()); internal::MachPortAttachmentMac::WireFormat format = mach_port_attachment->GetWireFormat(destination_process); bool success = diff --git a/ipc/attachment_broker_unprivileged_mac.h b/ipc/attachment_broker_unprivileged_mac.h index 26cf443..b47233f 100644 --- a/ipc/attachment_broker_unprivileged_mac.h +++ b/ipc/attachment_broker_unprivileged_mac.h @@ -22,8 +22,9 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedMac ~AttachmentBrokerUnprivilegedMac() override; // IPC::AttachmentBroker overrides. - bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) override; + bool SendAttachmentToProcess( + const scoped_refptr<IPC::BrokerableAttachment>& attachment, + base::ProcessId destination_process) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; diff --git a/ipc/attachment_broker_unprivileged_win.cc b/ipc/attachment_broker_unprivileged_win.cc index e1d9941..9e9e7b2 100644 --- a/ipc/attachment_broker_unprivileged_win.cc +++ b/ipc/attachment_broker_unprivileged_win.cc @@ -17,12 +17,12 @@ AttachmentBrokerUnprivilegedWin::AttachmentBrokerUnprivilegedWin() {} AttachmentBrokerUnprivilegedWin::~AttachmentBrokerUnprivilegedWin() {} bool AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess( - BrokerableAttachment* attachment, + const scoped_refptr<BrokerableAttachment>& attachment, base::ProcessId destination_process) { switch (attachment->GetBrokerableType()) { case BrokerableAttachment::WIN_HANDLE: { const internal::HandleAttachmentWin* handle_attachment = - static_cast<const internal::HandleAttachmentWin*>(attachment); + static_cast<const internal::HandleAttachmentWin*>(attachment.get()); internal::HandleAttachmentWin::WireFormat format = handle_attachment->GetWireFormat(destination_process); return get_sender()->Send( diff --git a/ipc/attachment_broker_unprivileged_win.h b/ipc/attachment_broker_unprivileged_win.h index bcb1500..9fa15ee 100644 --- a/ipc/attachment_broker_unprivileged_win.h +++ b/ipc/attachment_broker_unprivileged_win.h @@ -22,8 +22,9 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedWin ~AttachmentBrokerUnprivilegedWin() override; // IPC::AttachmentBroker overrides. - bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) override; + bool SendAttachmentToProcess( + const scoped_refptr<BrokerableAttachment>& attachment, + base::ProcessId destination_process) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc index 85f4475..1f73e78 100644 --- a/ipc/ipc_channel_posix.cc +++ b/ipc/ipc_channel_posix.cc @@ -697,7 +697,7 @@ bool ChannelPosix::ProcessMessageForDelivery(Message* message) { if (message->HasBrokerableAttachments()) { DCHECK(GetAttachmentBroker()); DCHECK(peer_pid_ != base::kNullProcessId); - for (BrokerableAttachment* attachment : + for (const scoped_refptr<IPC::BrokerableAttachment>& attachment : message->attachment_set()->GetBrokerableAttachments()) { if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment, peer_pid_)) { diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc index 3f3a56e..a82093e 100644 --- a/ipc/ipc_channel_reader.cc +++ b/ipc/ipc_channel_reader.cc @@ -246,9 +246,9 @@ ChannelReader::AttachmentIdSet ChannelReader::GetBrokeredAttachments( #if USE_ATTACHMENT_BROKER MessageAttachmentSet* set = msg->attachment_set(); - std::vector<BrokerableAttachment*> brokerable_attachments_copy = - set->GetBrokerableAttachments(); - for (const BrokerableAttachment* attachment : brokerable_attachments_copy) { + std::vector<scoped_refptr<IPC::BrokerableAttachment>> + brokerable_attachments_copy(set->GetBrokerableAttachments()); + for (const auto& attachment : brokerable_attachments_copy) { if (attachment->NeedsBrokering()) { AttachmentBroker* broker = GetAttachmentBroker(); DCHECK(broker); diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc index c528996..fdf3144 100644 --- a/ipc/ipc_channel_reader_unittest.cc +++ b/ipc/ipc_channel_reader_unittest.cc @@ -42,8 +42,9 @@ class MockAttachmentBroker : public AttachmentBroker { public: typedef std::set<scoped_refptr<BrokerableAttachment>> AttachmentSet; - bool SendAttachmentToProcess(BrokerableAttachment* attachment, - base::ProcessId destination_process) override { + bool SendAttachmentToProcess( + const scoped_refptr<BrokerableAttachment>& attachment, + base::ProcessId destination_process) override { return false; } diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc index 2a487ff..19f7cb5 100644 --- a/ipc/ipc_channel_win.cc +++ b/ipc/ipc_channel_win.cc @@ -107,7 +107,7 @@ bool ChannelWin::ProcessMessageForDelivery(Message* message) { if (message->HasBrokerableAttachments()) { DCHECK(GetAttachmentBroker()); DCHECK(peer_pid_ != base::kNullProcessId); - for (BrokerableAttachment* attachment : + for (const scoped_refptr<IPC::BrokerableAttachment>& attachment : message->attachment_set()->GetBrokerableAttachments()) { if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment, peer_pid_)) { diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc index 2738bf1..d64c8028 100644 --- a/ipc/ipc_message.cc +++ b/ipc/ipc_message.cc @@ -147,16 +147,15 @@ Message::NextMessageInfo::~NextMessageInfo() {} Message::SerializedAttachmentIds Message::SerializedIdsOfBrokerableAttachments() { DCHECK(HasBrokerableAttachments()); - std::vector<BrokerableAttachment*> attachments = - attachment_set_->GetBrokerableAttachments(); + std::vector<scoped_refptr<IPC::BrokerableAttachment>> attachments( + attachment_set_->GetBrokerableAttachments()); CHECK_LE(attachments.size(), std::numeric_limits<size_t>::max() / BrokerableAttachment::kNonceSize); size_t size = attachments.size() * BrokerableAttachment::kNonceSize; char* buffer = static_cast<char*>(malloc(size)); for (size_t i = 0; i < attachments.size(); ++i) { - const BrokerableAttachment* attachment = attachments[i]; char* start_range = buffer + i * BrokerableAttachment::kNonceSize; - BrokerableAttachment::AttachmentId id = attachment->GetIdentifier(); + BrokerableAttachment::AttachmentId id = attachments[i]->GetIdentifier(); id.SerializeToBuffer(start_range, BrokerableAttachment::kNonceSize); } SerializedAttachmentIds ids; diff --git a/ipc/ipc_message_attachment_set.cc b/ipc/ipc_message_attachment_set.cc index b290e0a..16446c5 100644 --- a/ipc/ipc_message_attachment_set.cc +++ b/ipc/ipc_message_attachment_set.cc @@ -175,26 +175,18 @@ void MessageAttachmentSet::CommitAllDescriptors() { consumed_descriptor_highwater_ = 0; } -std::vector<BrokerableAttachment*> +std::vector<scoped_refptr<IPC::BrokerableAttachment>> MessageAttachmentSet::GetBrokerableAttachments() const { - std::vector<BrokerableAttachment*> output; - for (const scoped_refptr<MessageAttachment>& attachment : - brokerable_attachments_) { - output.push_back(static_cast<BrokerableAttachment*>(attachment.get())); - } - return output; + return brokerable_attachments_; } void MessageAttachmentSet::ReplacePlaceholderWithAttachment( const scoped_refptr<BrokerableAttachment>& attachment) { + DCHECK_NE(BrokerableAttachment::PLACEHOLDER, attachment->GetBrokerableType()); for (auto it = brokerable_attachments_.begin(); it != brokerable_attachments_.end(); ++it) { - BrokerableAttachment* brokerable_attachment = - static_cast<BrokerableAttachment*>(it->get()); - - if (brokerable_attachment->GetBrokerableType() == - BrokerableAttachment::PLACEHOLDER && - brokerable_attachment->GetIdentifier() == attachment->GetIdentifier()) { + if ((*it)->GetBrokerableType() == BrokerableAttachment::PLACEHOLDER && + (*it)->GetIdentifier() == attachment->GetIdentifier()) { *it = attachment; return; } diff --git a/ipc/ipc_message_attachment_set.h b/ipc/ipc_message_attachment_set.h index 4276fcd..6a94b19 100644 --- a/ipc/ipc_message_attachment_set.h +++ b/ipc/ipc_message_attachment_set.h @@ -87,7 +87,8 @@ class IPC_EXPORT MessageAttachmentSet void CommitAllDescriptors(); // Returns a vector of all brokerable attachments. - std::vector<BrokerableAttachment*> GetBrokerableAttachments() const; + std::vector<scoped_refptr<IPC::BrokerableAttachment>> + GetBrokerableAttachments() const; // Replaces a placeholder brokerable attachment with |attachment|, matching // them by their id. |