Bug 1516578 Part 3 - Use UniquePtr more for web replay messages, r=mccr8.
authorBrian Hackett <bhackett1024@gmail.com>
Thu, 27 Dec 2018 13:28:49 -1000
changeset 453684 a2018dda265485061f8df13f14ee856af7613f95
parent 453683 a42a0f3a48744666791f1827c7f57488cec85fa9
child 453685 5dcb44dc558d9b45f42f0b20c646251d0ee4c255
push id111123
push userbhackett@mozilla.com
push dateSun, 13 Jan 2019 22:13:26 +0000
treeherdermozilla-inbound@f7361bad8498 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1516578
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1516578 Part 3 - Use UniquePtr more for web replay messages, r=mccr8.
toolkit/recordreplay/ipc/Channel.cpp
toolkit/recordreplay/ipc/Channel.h
toolkit/recordreplay/ipc/ChildIPC.cpp
toolkit/recordreplay/ipc/ChildProcess.cpp
--- a/toolkit/recordreplay/ipc/Channel.cpp
+++ b/toolkit/recordreplay/ipc/Channel.cpp
@@ -130,21 +130,21 @@ Channel::Channel(size_t aId, bool aMiddl
 
   {
     MonitorAutoLock lock(channel->mMonitor);
     channel->mInitialized = true;
     channel->mMonitor.Notify();
   }
 
   while (true) {
-    Message* msg = channel->WaitForMessage();
+    Message::UniquePtr msg = channel->WaitForMessage();
     if (!msg) {
       break;
     }
-    channel->mHandler(msg);
+    channel->mHandler(std::move(msg));
   }
 }
 
 void Channel::SendMessage(const Message& aMsg) {
   MOZ_RELEASE_ASSERT(NS_IsMainThread() ||
                      aMsg.mType == MessageType::BeginFatalError ||
                      aMsg.mType == MessageType::FatalError ||
                      aMsg.mType == MessageType::MiddlemanCallRequest);
@@ -170,17 +170,17 @@ void Channel::SendMessage(const Message&
       MOZ_RELEASE_ASSERT(errno == EPIPE);
       return;
     }
     ptr += rv;
     nbytes -= rv;
   }
 }
 
-Message* Channel::WaitForMessage() {
+Message::UniquePtr Channel::WaitForMessage() {
   if (!mMessageBuffer) {
     mMessageBuffer = (MessageBuffer*)AllocateMemory(sizeof(MessageBuffer),
                                                     MemoryKind::Generic);
     mMessageBuffer->appendN(0, PageSize);
   }
 
   size_t messageSize = 0;
   while (true) {
@@ -211,17 +211,17 @@ Message* Channel::WaitForMessage() {
       }
       PrintSpew("Channel disconnected, exiting...\n");
       _exit(0);
     }
 
     mMessageBytes += nbytes;
   }
 
-  Message* res = ((Message*)mMessageBuffer->begin())->Clone();
+  Message::UniquePtr res = ((Message*)mMessageBuffer->begin())->Clone();
 
   // Remove the message we just received from the incoming buffer.
   size_t remaining = mMessageBytes - messageSize;
   if (remaining) {
     memmove(mMessageBuffer->begin(), &mMessageBuffer->begin()[messageSize],
             remaining);
   }
   mMessageBytes = remaining;
--- a/toolkit/recordreplay/ipc/Channel.h
+++ b/toolkit/recordreplay/ipc/Channel.h
@@ -6,16 +6,17 @@
 
 #ifndef mozilla_recordreplay_Channel_h
 #define mozilla_recordreplay_Channel_h
 
 #include "base/process.h"
 
 #include "mozilla/gfx/Types.h"
 #include "mozilla/Maybe.h"
+#include "mozilla/UniquePtr.h"
 
 #include "File.h"
 #include "JSControl.h"
 #include "MiddlemanCall.h"
 #include "Monitor.h"
 
 namespace mozilla {
 namespace recordreplay {
@@ -155,20 +156,23 @@ struct Message {
   uint32_t mSize;
 
  protected:
   Message(MessageType aType, uint32_t aSize) : mType(aType), mSize(aSize) {
     MOZ_RELEASE_ASSERT(mSize >= sizeof(*this));
   }
 
  public:
-  Message* Clone() const {
-    char* res = (char*)malloc(mSize);
+  struct FreePolicy { void operator()(Message* msg) { /*free(msg);*/ } };
+  typedef UniquePtr<Message, FreePolicy> UniquePtr;
+
+  UniquePtr Clone() const {
+    Message* res = static_cast<Message*>(malloc(mSize));
     memcpy(res, this, mSize);
-    return (Message*)res;
+    return UniquePtr(res);
   }
 
   const char* TypeString() const {
     switch (mType) {
 #define EnumToString(Kind) \
   case MessageType::Kind:  \
     return #Kind;
       ForEachMessageType(EnumToString)
@@ -434,17 +438,17 @@ static inline MiddlemanCallResponseMessa
   return MiddlemanCallResponseMessage::New(outputData.begin(),
                                            outputData.length());
 }
 
 class Channel {
  public:
   // Note: the handler is responsible for freeing its input message. It will be
   // called on the channel's message thread.
-  typedef std::function<void(Message*)> MessageHandler;
+  typedef std::function<void(Message::UniquePtr)> MessageHandler;
 
  private:
   // ID for this channel, unique for the middleman.
   size_t mId;
 
   // Callback to invoke off thread on incoming messages.
   MessageHandler mHandler;
 
@@ -468,17 +472,17 @@ class Channel {
   // The number of bytes of data already in the message buffer.
   size_t mMessageBytes;
 
   // If spew is enabled, print a message and associated info to stderr.
   void PrintMessage(const char* aPrefix, const Message& aMsg);
 
   // Block until a complete message is received from the other side of the
   // channel.
-  Message* WaitForMessage();
+  Message::UniquePtr WaitForMessage();
 
   // Main routine for the channel's thread.
   static void ThreadMain(void* aChannel);
 
  public:
   // Initialize this channel, connect to the other side, and spin up a thread
   // to process incoming messages by calling aHandler.
   Channel(size_t aId, bool aMiddlemanRecording, const MessageHandler& aHandler);
--- a/toolkit/recordreplay/ipc/ChildIPC.cpp
+++ b/toolkit/recordreplay/ipc/ChildIPC.cpp
@@ -53,36 +53,38 @@ static StaticInfallibleVector<char*> gPa
 
 // File descriptors used by a pipe to create checkpoints when instructed by the
 // parent process.
 static FileHandle gCheckpointWriteFd;
 static FileHandle gCheckpointReadFd;
 
 // Copy of the introduction message we got from the middleman. This is saved on
 // receipt and then processed during InitRecordingOrReplayingProcess.
-static IntroductionMessage* gIntroductionMessage;
+static UniquePtr<IntroductionMessage, Message::FreePolicy> gIntroductionMessage;
 
 // When recording, whether developer tools server code runs in the middleman.
 static bool gDebuggerRunsInMiddleman;
 
 // Any response received to the last MiddlemanCallRequest message.
-static MiddlemanCallResponseMessage* gCallResponseMessage;
+static UniquePtr<MiddlemanCallResponseMessage, Message::FreePolicy>
+  gCallResponseMessage;
 
 // Whether some thread has sent a MiddlemanCallRequest and is waiting for
 // gCallResponseMessage to be filled in.
 static bool gWaitingForCallResponse;
 
 // Processing routine for incoming channel messages.
-static void ChannelMessageHandler(Message* aMsg) {
+static void ChannelMessageHandler(Message::UniquePtr aMsg) {
   MOZ_RELEASE_ASSERT(MainThreadShouldPause() || aMsg->CanBeSentWhileUnpaused());
 
   switch (aMsg->mType) {
     case MessageType::Introduction: {
       MOZ_RELEASE_ASSERT(!gIntroductionMessage);
-      gIntroductionMessage = (IntroductionMessage*)aMsg->Clone();
+      gIntroductionMessage.reset(
+          static_cast<IntroductionMessage*>(aMsg.release()));
       break;
     }
     case MessageType::CreateCheckpoint: {
       MOZ_RELEASE_ASSERT(IsRecording());
 
       // Ignore requests to create checkpoints before we have reached the first
       // paint and finished initializing.
       if (navigation::IsInitialized()) {
@@ -172,26 +174,24 @@ static void ChannelMessageHandler(Messag
       PauseMainThreadAndInvokeCallback(
           [=]() { navigation::RunToPoint(nmsg.mTarget); });
       break;
     }
     case MessageType::MiddlemanCallResponse: {
       MonitorAutoLock lock(*gMonitor);
       MOZ_RELEASE_ASSERT(gWaitingForCallResponse);
       MOZ_RELEASE_ASSERT(!gCallResponseMessage);
-      gCallResponseMessage = (MiddlemanCallResponseMessage*)aMsg;
-      aMsg = nullptr;  // Avoid freeing the message below.
+      gCallResponseMessage.reset(
+          static_cast<MiddlemanCallResponseMessage*>(aMsg.release()));
       gMonitor->NotifyAll();
       break;
     }
     default:
       MOZ_CRASH();
   }
-
-  free(aMsg);
 }
 
 // Main routine for a thread whose sole purpose is to listen to requests from
 // the middleman process to create a new checkpoint. This is separate from the
 // channel thread because this thread is recorded and the latter is not
 // recorded. By communicating between the two threads with a pipe, this
 // thread's behavior will be replicated exactly when replaying and new
 // checkpoints will be created at the same point as during recording.
@@ -317,17 +317,16 @@ void InitRecordingOrReplayingProcess(int
     for (size_t i = 0; i < msg->mArgc; i++) {
       gParentArgv.append(strdup(pos));
       pos += strlen(pos) + 1;
     }
 
     free(msg);
   }
 
-  free(gIntroductionMessage);
   gIntroductionMessage = nullptr;
 
   // Some argument manipulation code expects a null pointer at the end.
   gParentArgv.append(nullptr);
 
   MOZ_RELEASE_ASSERT(*aArgc >= 1);
   MOZ_RELEASE_ASSERT(gParentArgv.back() == nullptr);
 
@@ -698,17 +697,16 @@ void SendMiddlemanCallRequest(const char
 
   while (!gCallResponseMessage) {
     gMonitor->Wait();
   }
 
   aOutputData->append(gCallResponseMessage->BinaryData(),
                       gCallResponseMessage->BinaryDataSize());
 
-  free(gCallResponseMessage);
   gCallResponseMessage = nullptr;
   gWaitingForCallResponse = false;
 
   gMonitor->Notify();
 }
 
 void SendResetMiddlemanCalls() {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
--- a/toolkit/recordreplay/ipc/ChildProcess.cpp
+++ b/toolkit/recordreplay/ipc/ChildProcess.cpp
@@ -421,19 +421,20 @@ void GetArgumentsForChildProcess(base::P
 void ChildProcessInfo::LaunchSubprocess(
     const Maybe<RecordingProcessData>& aRecordingProcessData) {
   size_t channelId = gNumChannels++;
 
   // Create a new channel every time we launch a new subprocess, without
   // deleting or tearing down the old one's state. This is pretty lame and it
   // would be nice if we could do something better here, especially because
   // with restarts we could create any number of channels over time.
-  mChannel = new Channel(channelId, IsRecording(), [=](Message* aMsg) {
-    ReceiveChildMessageOnMainThread(channelId, aMsg);
-  });
+  mChannel = new Channel(channelId, IsRecording(),
+                         [=](Message::UniquePtr aMsg) {
+                           ReceiveChildMessageOnMainThread(std::move(aMsg));
+                         });
 
   MOZ_RELEASE_ASSERT(IsRecording() == aRecordingProcessData.isSome());
   if (IsRecording()) {
     std::vector<std::string> extraArgs;
     GetArgumentsForChildProcess(base::GetCurrentProcId(), channelId,
                                 gRecordingFilename, /* aRecording = */ true,
                                 extraArgs);