Bug 1516578 Part 3 - Use UniquePtr more for web replay messages, r=mccr8.
☠☠ backed out by 9e3564442734 ☠ ☠
authorBrian Hackett <bhackett1024@gmail.com>
Thu, 27 Dec 2018 13:28:49 -1000
changeset 453654 fb64ff37f6345cfa2e8efc7d692bc3dc3a1cd6f5
parent 453653 0426a61d27a9c9c047b4d489e4a2586b4c7a6491
child 453655 9ddc5bc1e961af52dbd772f66559f3fe4199e572
push id35365
push userdvarga@mozilla.com
push dateSun, 13 Jan 2019 10:05:55 +0000
treeherdermozilla-central@1218e374fbc7 [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);