From 1ffaefe5093e0f96d7555dbacd533d2d5b459864 Mon Sep 17 00:00:00 2001 From: Andrew McCreight Date: Tue, 2 Jun 2020 21:25:27 +0000 Subject: [PATCH] Bug 1642738, part 1 - Make the non-reply MessageChannel Send messages take a UniquePtr. r=froydnj This fixes the leak of a ShmemCreated in the case where ShareHandle fails. Differential Revision: https://phabricator.services.mozilla.com/D77907 --- ipc/glue/MessageChannel.cpp | 17 ++++++++--------- ipc/glue/MessageChannel.h | 6 +++--- ipc/glue/ProtocolUtils.cpp | 11 +++++------ ipc/glue/ProtocolUtils.h | 2 +- ipc/glue/Shmem.cpp | 14 ++++++++------ ipc/glue/Shmem.h | 7 ++++--- 6 files changed, 29 insertions(+), 28 deletions(-) diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp index c07892ac8b94..ca1608c2f8b5 100644 --- a/ipc/glue/MessageChannel.cpp +++ b/ipc/glue/MessageChannel.cpp @@ -941,7 +941,7 @@ bool MessageChannel::OpenOnSameThread(MessageChannel* aTargetChan, return true; } -bool MessageChannel::Send(Message* aMsg) { +bool MessageChannel::Send(UniquePtr aMsg) { if (aMsg->size() >= kMinTelemetryMessageSize) { Telemetry::Accumulate(Telemetry::IPC_MESSAGE_SIZE2, aMsg->size()); } @@ -963,28 +963,27 @@ bool MessageChannel::Send(Message* aMsg) { MOZ_RELEASE_ASSERT(!aMsg->is_sync()); MOZ_RELEASE_ASSERT(aMsg->nested_level() != IPC::Message::NESTED_INSIDE_SYNC); - CxxStackFrame frame(*this, OUT_MESSAGE, aMsg); + CxxStackFrame frame(*this, OUT_MESSAGE, aMsg.get()); - UniquePtr msg(aMsg); AssertWorkerThread(); mMonitor->AssertNotCurrentThreadOwns(); - if (MSG_ROUTING_NONE == msg->routing_id()) { + if (MSG_ROUTING_NONE == aMsg->routing_id()) { ReportMessageRouteError("MessageChannel::Send"); return false; } - if (msg->seqno() == 0) { - msg->set_seqno(NextSeqno()); + if (aMsg->seqno() == 0) { + aMsg->set_seqno(NextSeqno()); } MonitorAutoLock lock(*mMonitor); if (!Connected()) { - ReportConnectionError("MessageChannel", msg.get()); + ReportConnectionError("MessageChannel", aMsg.get()); return false; } - AddProfilerMarker(msg.get(), MessageDirection::eSending); - SendMessageToLink(std::move(msg)); + AddProfilerMarker(aMsg.get(), MessageDirection::eSending); + SendMessageToLink(std::move(aMsg)); return true; } diff --git a/ipc/glue/MessageChannel.h b/ipc/glue/MessageChannel.h index 5b2aecf870d7..994b1404e062 100644 --- a/ipc/glue/MessageChannel.h +++ b/ipc/glue/MessageChannel.h @@ -218,16 +218,16 @@ class MessageChannel : HasResultCodes, MessageLoop::DestructionObserver { ChannelFlags GetChannelFlags() { return mFlags; } // Asynchronously send a message to the other side of the channel - bool Send(Message* aMsg); + bool Send(UniquePtr aMsg); // Asynchronously send a message to the other side of the channel // and wait for asynchronous reply. template - void Send(Message* aMsg, ActorIdType aActorId, + void Send(UniquePtr aMsg, ActorIdType aActorId, ResolveCallback&& aResolve, RejectCallback&& aReject) { int32_t seqno = NextSeqno(); aMsg->set_seqno(seqno); - if (!Send(aMsg)) { + if (!Send(std::move(aMsg))) { aReject(ResponseRejectReason::SendError); return; } diff --git a/ipc/glue/ProtocolUtils.cpp b/ipc/glue/ProtocolUtils.cpp index 1cb4a15df171..0feca60295ff 100644 --- a/ipc/glue/ProtocolUtils.cpp +++ b/ipc/glue/ProtocolUtils.cpp @@ -463,7 +463,7 @@ bool IProtocol::ChannelSend(IPC::Message* aMsg) { // NOTE: This send call failing can only occur during toplevel channel // teardown. As this is an async call, this isn't reasonable to predict or // respond to, so just drop the message on the floor silently. - GetIPCChannel()->Send(msg.release()); + GetIPCChannel()->Send(std::move(msg)); return true; } @@ -699,12 +699,12 @@ Shmem::SharedMemory* IToplevelProtocol::CreateSharedMemory( OtherPid(); #endif - Message* descriptor = + UniquePtr descriptor = shmem.ShareTo(Shmem::PrivateIPDLCaller(), pid, MSG_ROUTING_CONTROL); if (!descriptor) { return nullptr; } - Unused << GetIPCChannel()->Send(descriptor); + Unused << GetIPCChannel()->Send(std::move(descriptor)); *aId = shmem.Id(Shmem::PrivateIPDLCaller()); Shmem::SharedMemory* rawSegment = segment.get(); @@ -733,7 +733,7 @@ bool IToplevelProtocol::DestroySharedMemory(Shmem& shmem) { return false; } - Message* descriptor = + UniquePtr descriptor = shmem.UnshareFrom(Shmem::PrivateIPDLCaller(), MSG_ROUTING_CONTROL); MOZ_ASSERT(mShmemMap.Contains(aId), @@ -743,11 +743,10 @@ bool IToplevelProtocol::DestroySharedMemory(Shmem& shmem) { MessageChannel* channel = GetIPCChannel(); if (!channel->CanSend()) { - delete descriptor; return true; } - return descriptor && channel->Send(descriptor); + return descriptor && channel->Send(std::move(descriptor)); } void IToplevelProtocol::DeallocShmems() { diff --git a/ipc/glue/ProtocolUtils.h b/ipc/glue/ProtocolUtils.h index 648a6580567e..d80a7eec5292 100644 --- a/ipc/glue/ProtocolUtils.h +++ b/ipc/glue/ProtocolUtils.h @@ -289,7 +289,7 @@ class IProtocol : public HasResultCodes { RejectCallback&& aReject) { UniquePtr msg(aMsg); if (CanSend()) { - GetIPCChannel()->Send(msg.release(), this, std::move(aResolve), + GetIPCChannel()->Send(std::move(msg), this, std::move(aResolve), std::move(aReject)); } else { NS_WARNING("IPC message discarded: actor cannot send"); diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp index a043ad94b2c8..c0a633b2613d 100644 --- a/ipc/glue/Shmem.cpp +++ b/ipc/glue/Shmem.cpp @@ -416,12 +416,13 @@ void Shmem::Dealloc(PrivateIPDLCaller, SharedMemory* aSegment) { #endif // if defined(DEBUG) -IPC::Message* Shmem::ShareTo(PrivateIPDLCaller, base::ProcessId aTargetPid, - int32_t routingId) { +UniquePtr Shmem::ShareTo(PrivateIPDLCaller, + base::ProcessId aTargetPid, + int32_t routingId) { AssertInvariants(); - IPC::Message* msg = new ShmemCreated(routingId, mId, mSize, mSegment->Type()); - if (!mSegment->ShareHandle(aTargetPid, msg)) { + auto msg = MakeUnique(routingId, mId, mSize, mSegment->Type()); + if (!mSegment->ShareHandle(aTargetPid, msg.get())) { return nullptr; } // close the handle to the segment after it is shared @@ -429,9 +430,10 @@ IPC::Message* Shmem::ShareTo(PrivateIPDLCaller, base::ProcessId aTargetPid, return msg; } -IPC::Message* Shmem::UnshareFrom(PrivateIPDLCaller, int32_t routingId) { +UniquePtr Shmem::UnshareFrom(PrivateIPDLCaller, + int32_t routingId) { AssertInvariants(); - return new ShmemDestroyed(routingId, mId); + return MakeUnique(routingId, mId); } void IPDLParamTraits::Write(IPC::Message* aMsg, IProtocol* aActor, diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h index cca75eee1084..314ba0379f42 100644 --- a/ipc/glue/Shmem.h +++ b/ipc/glue/Shmem.h @@ -18,6 +18,7 @@ #include "ipc/IPCMessageUtils.h" #include "mozilla/ipc/SharedMemory.h" #include "mozilla/ipc/IPDLParamTraits.h" +#include "mozilla/UniquePtr.h" /** * |Shmem| is one agent in the IPDL shared memory scheme. The way it @@ -158,14 +159,14 @@ class Shmem final { // that contains enough information for the other process to map // this segment in OpenExisting() below. Return a new message if // successful (owned by the caller), nullptr if not. - IPC::Message* ShareTo(PrivateIPDLCaller, base::ProcessId aTargetPid, - int32_t routingId); + UniquePtr ShareTo(PrivateIPDLCaller, base::ProcessId aTargetPid, + int32_t routingId); // Stop sharing this with |aTargetPid|. Return an IPC message that // contains enough information for the other process to unmap this // segment. Return a new message if successful (owned by the // caller), nullptr if not. - IPC::Message* UnshareFrom(PrivateIPDLCaller, int32_t routingId); + UniquePtr UnshareFrom(PrivateIPDLCaller, int32_t routingId); // Return a SharedMemory instance in this process using the // descriptor shared to us by the process that created the