Bug 1779367 - Don't send shutdown profiles that don't fit in an IPC message - r=florian
authorGerald Squelart <gsquelart@mozilla.com>
Wed, 20 Jul 2022 12:53:00 +0000
changeset 624510 a58be620f788efee642fb80f45240ef571d275ad
parent 624509 2ae1019d8ff76d10060ff043d033a10949e949fc
child 624511 70cacc19a882ebf0f7545b5eaa071662b670e6b2
push id40007
push usersmolnar@mozilla.com
push dateWed, 20 Jul 2022 21:52:02 +0000
treeherdermozilla-central@16a4302fb1a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersflorian
bugs1779367
milestone104.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 1779367 - Don't send shutdown profiles that don't fit in an IPC message - r=florian Instead of trying to send a too-big message, send a short message starting with '*', which the parent can put into the profileGatheringLog. Note: The `CollectProfileOrEmptyString` is now only used by GrabShutdownProfile (it was previously also used by RecvGatherProfile before it went async). Since this patch is completely changing its implementation, we may as well fold it back into its only caller. Differential Revision: https://phabricator.services.mozilla.com/D152027
dom/ipc/ContentChild.cpp
dom/ipc/PContent.ipdl
tools/profiler/core/platform.cpp
tools/profiler/gecko/ProfilerChild.cpp
tools/profiler/gecko/nsProfiler.cpp
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -3024,16 +3024,23 @@ void ContentChild::ShutdownInternal() {
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
         isProfiling ? "Profiling - Destroying ChildProfilerController"_ns
                     : "Not profiling - Destroying ChildProfilerController"_ns);
     mProfilerController = nullptr;
     CrashReporter::AnnotateCrashReport(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
         isProfiling ? "Profiling - SendShutdownProfile (sending)"_ns
                     : "Not profiling - SendShutdownProfile (sending)"_ns);
+    if (const size_t len = shutdownProfile.Length();
+        len >= size_t(IPC::Channel::kMaximumMessageSize)) {
+      shutdownProfile = nsPrintfCString(
+          "*Profile from pid %u bigger (%zu) than IPC max (%zu)",
+          unsigned(profiler_current_process_id().ToNumber()), len,
+          size_t(IPC::Channel::kMaximumMessageSize));
+    }
     // Send the shutdown profile to the parent process through our own
     // message channel, which we know will survive for long enough.
     bool sent = SendShutdownProfile(shutdownProfile);
     CrashReporter::AnnotateCrashReport(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
         sent ? (isProfiling ? "Profiling - SendShutdownProfile (sent)"_ns
                             : "Not profiling - SendShutdownProfile (sent)"_ns)
              : (isProfiling
--- a/dom/ipc/PContent.ipdl
+++ b/dom/ipc/PContent.ipdl
@@ -1310,16 +1310,24 @@ parent:
      */
     async PContentPermissionRequest(PermissionRequest[] aRequests,
                                     Principal aPrincipal,
                                     Principal aTopLevelPrincipal,
                                     bool aIsHandlingUserInput,
                                     bool aMaybeUnsafePermissionDelegate,
                                     TabId tabId);
 
+    /**
+     * If the profiler is running when the process shuts down, this sends the
+     * profile data collected so far.
+     *
+     * @param aProfile
+     *   This may contain an empty string (unknown issue), an error message
+     *   starting with '*', or a profile as a stringified JSON object.
+     */
     async ShutdownProfile(nsCString aProfile);
 
     /**
      * This sends any collected perf stats data on shutdown.
      */
     async ShutdownPerfStats(nsCString aPerfStats);
 
     /**
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -5512,17 +5512,17 @@ static void locked_profiler_save_profile
     {
       locked_profiler_stream_json_for_this_process(
           aLock, w, /* sinceTime */ 0, aPreRecordedMetaInformation,
           aIsShuttingDown, nullptr, ProgressLogger{});
 
       w.StartArrayProperty("processes");
       Vector<nsCString> exitProfiles = ActivePS::MoveExitProfiles(aLock);
       for (auto& exitProfile : exitProfiles) {
-        if (!exitProfile.IsEmpty()) {
+        if (!exitProfile.IsEmpty() && exitProfile[0] != '*') {
           w.Splice(exitProfile);
         }
       }
       w.EndArray();
     }
     w.End();
 
     stream.close();
--- a/tools/profiler/gecko/ProfilerChild.cpp
+++ b/tools/profiler/gecko/ProfilerChild.cpp
@@ -3,19 +3,22 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ProfilerChild.h"
 
 #include "GeckoProfiler.h"
 #include "platform.h"
+#include "ProfilerCodeAddressService.h"
 #include "ProfilerControl.h"
 #include "ProfilerParent.h"
 
+#include "chrome/common/ipc_channel.h"
+#include "nsPrintfCString.h"
 #include "nsThreadUtils.h"
 
 #include <memory>
 
 namespace mozilla {
 
 /* static */ DataMutexBase<ProfilerChild::ProfilerChildAndUpdate,
                            baseprofiler::detail::BaseProfilerMutex>
@@ -251,27 +254,16 @@ mozilla::ipc::IPCResult ProfilerChild::R
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ProfilerChild::RecvClearAllPages() {
   profiler_clear_all_pages();
   return IPC_OK();
 }
 
-static nsCString CollectProfileOrEmptyString(bool aIsShuttingDown) {
-  nsCString profileCString;
-  UniquePtr<char[]> profile =
-      profiler_get_profile(/* aSinceTime */ 0, aIsShuttingDown);
-  if (profile) {
-    size_t len = strlen(profile.get());
-    profileCString.Adopt(profile.release(), len);
-  }
-  return profileCString;
-}
-
 mozilla::ipc::IPCResult ProfilerChild::RecvAwaitNextChunkManagerUpdate(
     AwaitNextChunkManagerUpdateResolver&& aResolve) {
   MOZ_ASSERT(!mDestroyed,
              "Recv... should not be called if the actor was destroyed");
   // Pick up pending updates if any.
   {
     auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
     if (lockedUpdate->mProfilerChild && !lockedUpdate->mUpdate.IsNotUpdate()) {
@@ -450,12 +442,66 @@ void ProfilerChild::ActorDestroy(ActorDe
 void ProfilerChild::Destroy() {
   ResetChunkManager();
   if (!mDestroyed) {
     Close();
   }
 }
 
 nsCString ProfilerChild::GrabShutdownProfile() {
-  return CollectProfileOrEmptyString(/* aIsShuttingDown */ true);
+  LOG("GrabShutdownProfile");
+
+  UniquePtr<ProfilerCodeAddressService> service =
+      profiler_code_address_service_for_presymbolication();
+  SpliceableChunkedJSONWriter writer;
+  writer.Start();
+  if (!profiler_stream_json_for_this_process(writer, /* aSinceTime */ 0,
+                                             /* aIsShuttingDown */ true,
+                                             service.get(), ProgressLogger{})) {
+    return nsPrintfCString("*Profile unavailable for pid %u",
+                           unsigned(profiler_current_process_id().ToNumber()));
+  }
+  writer.StartArrayProperty("processes");
+  writer.EndArray();
+  writer.End();
+
+  const size_t len = writer.ChunkedWriteFunc().Length();
+  // This string is destined to be sent as a shutdown profile, which is limited
+  // by the maximum IPC message size.
+  // TODO: IPC to change to shmem (bug 1780330), raising this limit to
+  // JS::MaxStringLength.
+  if (len >= size_t(IPC::Channel::kMaximumMessageSize)) {
+    return nsPrintfCString(
+        "*Profile from pid %u bigger (%zu) than IPC max (%zu)",
+        unsigned(profiler_current_process_id().ToNumber()), len,
+        size_t(IPC::Channel::kMaximumMessageSize));
+  }
+
+  nsCString profileCString;
+  if (!profileCString.SetLength(len, fallible)) {
+    return nsPrintfCString(
+        "*Could not allocate %zu bytes for profile from pid %u", len,
+        unsigned(profiler_current_process_id().ToNumber()));
+  }
+  MOZ_ASSERT(*(profileCString.Data() + len) == '\0',
+             "We expected a null at the end of the string buffer, to be "
+             "rewritten by CopyDataIntoLazilyAllocatedBuffer");
+
+  char* const profileBeginWriting = profileCString.BeginWriting();
+  if (!profileBeginWriting) {
+    return nsPrintfCString("*Could not write profile from pid %u",
+                           unsigned(profiler_current_process_id().ToNumber()));
+  }
+
+  // Here, we have enough space reserved in `profileCString`, starting at
+  // `profileBeginWriting`, copy the JSON profile there.
+  writer.ChunkedWriteFunc().CopyDataIntoLazilyAllocatedBuffer(
+      [&](size_t aBufferLen) -> char* {
+        MOZ_RELEASE_ASSERT(aBufferLen == len + 1);
+        return profileBeginWriting;
+      });
+  MOZ_ASSERT(*(profileCString.Data() + len) == '\0',
+             "We still expected a null at the end of the string buffer");
+
+  return profileCString;
 }
 
 }  // namespace mozilla
--- a/tools/profiler/gecko/nsProfiler.cpp
+++ b/tools/profiler/gecko/nsProfiler.cpp
@@ -1163,18 +1163,24 @@ RefPtr<nsProfiler::GatheringPromise> nsP
 
   mWriter->StartArrayProperty("processes");
 
   // If we have any process exit profiles, add them immediately.
   if (Vector<nsCString> exitProfiles = profiler_move_exit_profiles();
       !exitProfiles.empty()) {
     for (auto& exitProfile : exitProfiles) {
       if (!exitProfile.IsEmpty()) {
-        if (mWriter->ChunkedWriteFunc().Length() + exitProfile.Length() <
-            scLengthAccumulationThreshold) {
+        if (exitProfile[0] == '*') {
+          LogEvent([&](Json::Value& aEvent) {
+            aEvent.append(
+                Json::StaticString{"Exit non-profile with error message:"});
+            aEvent.append(exitProfile.Data() + 1);
+          });
+        } else if (mWriter->ChunkedWriteFunc().Length() + exitProfile.Length() <
+                   scLengthAccumulationThreshold) {
           mWriter->Splice(exitProfile);
           LogEvent([&](Json::Value& aEvent) {
             aEvent.append(Json::StaticString{"Added exit profile with size:"});
             aEvent.append(Json::Value::UInt64{exitProfile.Length()});
           });
         } else {
           LogEvent([&](Json::Value& aEvent) {
             aEvent.append(