Backed out 3 changesets (bug 1671701) for causing failure at browser_startup_mainthreadio.js. CLOSED TREE

Backed out changeset b4b6ec163792 (bug 1671701)
Backed out changeset a343865ccfe0 (bug 1671701)
Backed out changeset e6f882892fea (bug 1671701)

mozglue/baseprofiler/public/BaseProfilerMarkerTypes.h

@@ -48,6 +48,40 @@ struct Tracing {
   }
 };
 
+struct FileIO {
+  static constexpr Span<const char> MarkerTypeName() {
+    return MakeStringSpan("FileIO");
+  }
+  static void StreamJSONMarkerData(JSONWriter& aWriter,
+                                   const ProfilerString8View& aOperation,
+                                   const ProfilerString8View& aSource,
+                                   const ProfilerString8View& aFilename,
+                                   MarkerThreadId aOperationThreadId) {
+    aWriter.StringProperty("operation", aOperation);
+    aWriter.StringProperty("source", aSource);
+    if (aFilename.Length() != 0) {
+      aWriter.StringProperty("filename", aFilename);
+    }
+    if (!aOperationThreadId.IsUnspecified()) {
+      aWriter.IntProperty("threadId", aOperationThreadId.ThreadId());
+    }
+  }
+  static MarkerSchema MarkerTypeDisplay() {
+    using MS = MarkerSchema;
+    MS schema{MS::Location::markerChart, MS::Location::markerTable,
+              MS::Location::timelineFileIO};
+    schema.AddKeyLabelFormatSearchable("operation", "Operation",
+                                       MS::Format::string,
+                                       MS::Searchable::searchable);
+    schema.AddKeyLabelFormatSearchable("source", "Source", MS::Format::string,
+                                       MS::Searchable::searchable);
+    schema.AddKeyLabelFormatSearchable("filename", "Filename",
+                                       MS::Format::filePath,
+                                       MS::Searchable::searchable);
+    return schema;
+  }
+};
+
 struct UserTimingMark {
   static constexpr Span<const char> MarkerTypeName() {
     return MakeStringSpan("UserTimingMark");

mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h

@@ -477,11 +477,6 @@ class MarkerStack {
     return MarkerStack(true);
   }
 
-  // Optionally capture a stack, useful for avoiding long-winded ternaries.
-  static MarkerStack MaybeCapture(bool aDoCapture) {
-    return MarkerStack(aDoCapture);
-  }
-
   // Use an existing backtrace stored elsewhere, which the user must guarantee
   // is alive during the add-marker call. If empty, equivalent to NoStack().
   static MarkerStack UseBacktrace(

mozglue/tests/TestBaseProfiler.cpp

@@ -3368,6 +3368,19 @@ void TestProfiler() {
 
     // Just making sure all payloads know how to (de)serialize and stream.
 
+    baseprofiler::AddMarker("m2fileio", mozilla::baseprofiler::category::OTHER,
+                            {}, mozilla::baseprofiler::markers::FileIO{}, "op2",
+                            "src2", "f2", MarkerThreadId{});
+    baseprofiler::AddMarker(
+        "m2fileio-capture", mozilla::baseprofiler::category::OTHER,
+        MarkerStack::Capture(), mozilla::baseprofiler::markers::FileIO{}, "op2",
+        "src2", "f2", MarkerThreadId{});
+    baseprofiler::AddMarker(
+        "m2fileio-take-backtrace", mozilla::baseprofiler::category::OTHER,
+        MarkerStack::TakeBacktrace(baseprofiler::profiler_capture_backtrace()),
+        mozilla::baseprofiler::markers::FileIO{}, "op2", "src2", "f2",
+        MarkerThreadId{});
+
     MOZ_RELEASE_ASSERT(
         baseprofiler::AddMarker("markers 2.0 without options (omitted)",
                                 mozilla::baseprofiler::category::OTHER));
@@ -3521,6 +3534,7 @@ void TestProfiler() {
     // Check for some expected marker schema JSON output.
     MOZ_RELEASE_ASSERT(profileSV.find("\"markerSchema\": [") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"name\": \"Text\",") != svnpos);
+    MOZ_RELEASE_ASSERT(profileSV.find("\"name\": \"FileIO\",") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"name\": \"tracing\",") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"name\": \"UserTimingMark\",") !=
                        svnpos);
@@ -3535,6 +3549,7 @@ void TestProfiler() {
     MOZ_RELEASE_ASSERT(profileSV.find("\"display\": [") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"marker-chart\"") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"marker-table\"") != svnpos);
+    MOZ_RELEASE_ASSERT(profileSV.find("\"searchable\": true") != svnpos);
     MOZ_RELEASE_ASSERT(profileSV.find("\"format\": \"string\"") != svnpos);
     // TODO: Add more checks for what's expected in the profile. Some of them
     // are done in gtest's.

tools/profiler/core/ProfilerMarkerPayload.cpp

@@ -218,6 +218,52 @@ void TracingMarkerPayload::StreamPayload(SpliceableJSONWriter& aWriter,
   }
 }
 
+ProfileBufferEntryWriter::Length FileIOMarkerPayload::TagAndSerializationBytes()
+    const {
+  return CommonPropsTagAndSerializationBytes() +
+         ProfileBufferEntryWriter::SumBytes(
+             WrapProfileBufferRawPointer(mSource), mOperation, mFilename,
+             mIOThreadId);
+}
+
+void FileIOMarkerPayload::SerializeTagAndPayload(
+    ProfileBufferEntryWriter& aEntryWriter) const {
+  static const DeserializerTag tag = TagForDeserializer(Deserialize);
+  SerializeTagAndCommonProps(tag, aEntryWriter);
+  aEntryWriter.WriteObject(WrapProfileBufferRawPointer(mSource));
+  aEntryWriter.WriteObject(mOperation);
+  aEntryWriter.WriteObject(mFilename);
+  aEntryWriter.WriteObject(mIOThreadId);
+}
+
+// static
+UniquePtr<ProfilerMarkerPayload> FileIOMarkerPayload::Deserialize(
+    ProfileBufferEntryReader& aEntryReader) {
+  ProfilerMarkerPayload::CommonProps props =
+      DeserializeCommonProps(aEntryReader);
+  auto source = aEntryReader.ReadObject<const char*>();
+  auto operation = aEntryReader.ReadObject<UniqueFreePtr<char>>();
+  auto filename = aEntryReader.ReadObject<UniqueFreePtr<char>>();
+  auto ioThreadId = aEntryReader.ReadObject<Maybe<int>>();
+  return UniquePtr<ProfilerMarkerPayload>(
+      new FileIOMarkerPayload(std::move(props), source, std::move(operation),
+                              std::move(filename), ioThreadId));
+}
+
+void FileIOMarkerPayload::StreamPayload(SpliceableJSONWriter& aWriter,
+                                        const TimeStamp& aProcessStartTime,
+                                        UniqueStacks& aUniqueStacks) const {
+  StreamCommonProps("FileIO", aWriter, aProcessStartTime, aUniqueStacks);
+  aWriter.StringProperty("operation", MakeStringSpan(mOperation.get()));
+  aWriter.StringProperty("source", MakeStringSpan(mSource));
+  if (mFilename) {
+    aWriter.StringProperty("filename", MakeStringSpan(mFilename.get()));
+  }
+  if (mIOThreadId.isSome()) {
+    aWriter.IntProperty("threadId", *mIOThreadId);
+  }
+}
+
 ProfileBufferEntryWriter::Length
 UserTimingMarkerPayload::TagAndSerializationBytes() const {
   return CommonPropsTagAndSerializationBytes() +

tools/profiler/gecko/ProfilerIOInterposeObserver.cpp

@@ -3,46 +3,12 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ProfilerIOInterposeObserver.h"
+
 #include "GeckoProfiler.h"
+#include "ProfilerMarkerPayload.h"
 
 using namespace mozilla;
 
-namespace geckoprofiler::markers {
-struct FileIOMarker {
-  static constexpr Span<const char> MarkerTypeName() {
-    return MakeStringSpan("FileIO");
-  }
-  static void StreamJSONMarkerData(JSONWriter& aWriter,
-                                   const ProfilerString8View& aOperation,
-                                   const ProfilerString8View& aSource,
-                                   const ProfilerString8View& aFilename,
-                                   MarkerThreadId aOperationThreadId) {
-    aWriter.StringProperty("operation", aOperation);
-    aWriter.StringProperty("source", aSource);
-    if (aFilename.Length() != 0) {
-      aWriter.StringProperty("filename", aFilename);
-    }
-    if (!aOperationThreadId.IsUnspecified()) {
-      aWriter.IntProperty("threadId", aOperationThreadId.ThreadId());
-    }
-  }
-  static MarkerSchema MarkerTypeDisplay() {
-    using MS = MarkerSchema;
-    MS schema{MS::Location::markerChart, MS::Location::markerTable,
-              MS::Location::timelineFileIO};
-    schema.AddKeyLabelFormatSearchable("operation", "Operation",
-                                       MS::Format::string,
-                                       MS::Searchable::searchable);
-    schema.AddKeyLabelFormatSearchable("source", "Source", MS::Format::string,
-                                       MS::Searchable::searchable);
-    schema.AddKeyLabelFormatSearchable("filename", "Filename",
-                                       MS::Format::filePath,
-                                       MS::Searchable::searchable);
-    return schema;
-  }
-};
-}  // namespace geckoprofiler::markers
-
 static auto GetFilename(IOInterposeObserver::Observation& aObservation) {
   AUTO_PROFILER_STATS(IO_filename);
   constexpr size_t scExpectedMaxFilename = 512;
@@ -55,6 +21,14 @@ static auto GetFilename(IOInterposeObserver::Observation& aObservation) {
   return filename8;
 }
 
+static UniqueProfilerBacktrace GetBacktraceUnless(bool aPrevent) {
+  if (aPrevent) {
+    return nullptr;
+  }
+  AUTO_PROFILER_STATS(IO_backtrace);
+  return profiler_get_backtrace();
+}
+
 void ProfilerIOInterposeObserver::Observe(Observation& aObservation) {
   if (profiler_is_locked_on_current_thread()) {
     // Don't observe I/Os originating from the profiler itself (when internally
@@ -73,7 +47,6 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) {
     return;
   }
 
-  const bool doCaptureStack = !(features & ProfilerFeature::NoIOStacks);
   if (IsMainThread()) {
     // This is the main thread.
     // Capture a marker if any "IO" feature is on.
@@ -85,25 +58,12 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) {
     AUTO_PROFILER_STATS(IO_MT);
     nsAutoCString type{aObservation.FileType()};
     type.AppendLiteral("IO");
-
+    PROFILER_ADD_MARKER_WITH_PAYLOAD(
-    // Store the marker in the current thread.
+        type.get(), OTHER, FileIOMarkerPayload,
-    PROFILER_MARKER(
+        (aObservation.ObservedOperationString(), aObservation.Reference(),
-        type, OTHER,
+         GetFilename(aObservation).get(), aObservation.Start(),
-        MarkerOptions(
+         aObservation.End(),
-            MarkerTiming::Interval(aObservation.Start(), aObservation.End()),
+         GetBacktraceUnless(features & ProfilerFeature::NoIOStacks)));
-            MarkerStack::MaybeCapture(doCaptureStack)),
-        FileIOMarker,
-        // aOperation
-        ProfilerString8View::WrapNullTerminatedString(
-            aObservation.ObservedOperationString()),
-        // aSource
-        ProfilerString8View::WrapNullTerminatedString(aObservation.Reference()),
-        // aFilename
-        GetFilename(aObservation),
-        // aOperationThreadId - Do not include a thread ID, as it's the same as
-        // the markers. Only include this field when the marker is being sent
-        // from another thread.
-        MarkerThreadId{});
 
   } else if (profiler_thread_is_being_profiled()) {
     // This is a non-main thread that is being profiled.
@@ -111,57 +71,23 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) {
       return;
     }
     AUTO_PROFILER_STATS(IO_off_MT);
-
+    FileIOMarkerPayload payload{
+        aObservation.ObservedOperationString(),
+        aObservation.Reference(),
+        GetFilename(aObservation).get(),
+        aObservation.Start(),
+        aObservation.End(),
+        GetBacktraceUnless(features & ProfilerFeature::NoIOStacks)};
     nsAutoCString type{aObservation.FileType()};
     type.AppendLiteral("IO");
-
+    // Store the marker in the both:
-    // Share a backtrace between the marker on this thread, and the marker on
+    // - The current thread.
-    // the main thread.
+    profiler_add_marker(type.get(), JS::ProfilingCategoryPair::OTHER, payload);
-    UniquePtr<ProfileChunkedBuffer> backtrace =
+    // - The main thread (with a distinct marker name and the thread id).
-        doCaptureStack ? profiler_capture_backtrace() : nullptr;
+    payload.SetIOThreadId(profiler_current_thread_id());
-
-    // Store the marker in the current thread.
-    PROFILER_MARKER(
-        type, OTHER,
-        MarkerOptions(
-            MarkerTiming::Interval(aObservation.Start(), aObservation.End()),
-            backtrace ? MarkerStack::UseBacktrace(*backtrace)
-                      : MarkerStack::NoStack()),
-        FileIOMarker,
-        // aOperation
-        ProfilerString8View::WrapNullTerminatedString(
-            aObservation.ObservedOperationString()),
-        // aSource
-        ProfilerString8View::WrapNullTerminatedString(aObservation.Reference()),
-        // aFilename
-        GetFilename(aObservation),
-        // aOperationThreadId - Do not include a thread ID, as it's the same as
-        // the markers. Only include this field when the marker is being sent
-        // from another thread.
-        MarkerThreadId{});
-
-    // Store the marker in the main thread as well, with a distinct marker name
-    // and thread id.
     type.AppendLiteral(" (non-main thread)");
-    PROFILER_MARKER(
+    profiler_add_marker_for_mainthread(JS::ProfilingCategoryPair::OTHER,
-        type, OTHER,
+                                       type.get(), payload);
-        MarkerOptions(
-            MarkerTiming::Interval(aObservation.Start(), aObservation.End()),
-            backtrace ? MarkerStack::UseBacktrace(*backtrace)
-                      : MarkerStack::NoStack(),
-            // This is the important piece that changed.
-            // It will send a marker to the main thread.
-            MarkerThreadId(profiler_main_thread_id())),
-        FileIOMarker,
-        // aOperation
-        ProfilerString8View::WrapNullTerminatedString(
-            aObservation.ObservedOperationString()),
-        // aSource
-        ProfilerString8View::WrapNullTerminatedString(aObservation.Reference()),
-        // aFilename
-        GetFilename(aObservation),
-        // aOperationThreadId - Include the thread ID in the payload.
-        MarkerThreadId::CurrentThread());
 
   } else {
     // This is a thread that is not being profiled. We still want to capture
@@ -176,25 +102,13 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) {
     } else {
       type.AppendLiteral("IO (unregistered thread)");
     }
-
+    profiler_add_marker_for_mainthread(
-    // Only store this marker on the main thread, as this thread was not being
+        JS::ProfilingCategoryPair::OTHER, type.get(),
-    // profiled.
+        FileIOMarkerPayload(
-    PROFILER_MARKER(
+            aObservation.ObservedOperationString(), aObservation.Reference(),
-        type, OTHER,
+            GetFilename(aObservation).get(), aObservation.Start(),
-        MarkerOptions(
+            aObservation.End(),
-            MarkerTiming::Interval(aObservation.Start(), aObservation.End()),
+            GetBacktraceUnless(features & ProfilerFeature::NoIOStacks),
-            doCaptureStack ? MarkerStack::Capture() : MarkerStack::NoStack(),
+            Some(profiler_current_thread_id())));
-            // Store this marker on the main thread.
-            MarkerThreadId(profiler_main_thread_id())),
-        FileIOMarker,
-        // aOperation
-        ProfilerString8View::WrapNullTerminatedString(
-            aObservation.ObservedOperationString()),
-        // aSource
-        ProfilerString8View::WrapNullTerminatedString(aObservation.Reference()),
-        // aFilename
-        GetFilename(aObservation),
-        // aOperationThreadId - Note which thread this marker is coming from.
-        MarkerThreadId::CurrentThread());
   }
 }

tools/profiler/public/ProfilerMarkerPayload.h

@@ -249,6 +249,46 @@ class BudgetMarkerPayload : public ProfilerMarkerPayload {
       : ProfilerMarkerPayload(std::move(aCommonProps)) {}
 };
 
+class FileIOMarkerPayload : public ProfilerMarkerPayload {
+ public:
+  FileIOMarkerPayload(
+      const char* aOperation, const char* aSource, const char* aFilename,
+      const mozilla::TimeStamp& aStartTime, const mozilla::TimeStamp& aEndTime,
+      UniqueProfilerBacktrace aStack,
+      const mozilla::Maybe<int>& aIOThreadId = mozilla::Nothing())
+      : ProfilerMarkerPayload(aStartTime, aEndTime, mozilla::Nothing(),
+                              std::move(aStack)),
+        mSource(aSource),
+        mOperation(aOperation ? strdup(aOperation) : nullptr),
+        mFilename(aFilename ? strdup(aFilename) : nullptr),
+        mIOThreadId(aIOThreadId) {
+    MOZ_ASSERT(aSource);
+  }
+
+  // In some cases, the thread id needs to be set after the payload is created.
+  void SetIOThreadId(int aIOThreadId) {
+    mIOThreadId = mozilla::Some(aIOThreadId);
+  }
+
+  DECL_STREAM_PAYLOAD
+
+ private:
+  FileIOMarkerPayload(CommonProps&& aCommonProps, const char* aSource,
+                      mozilla::UniqueFreePtr<char>&& aOperation,
+                      mozilla::UniqueFreePtr<char>&& aFilename,
+                      const mozilla::Maybe<int>& aIOThreadId)
+      : ProfilerMarkerPayload(std::move(aCommonProps)),
+        mSource(aSource),
+        mOperation(std::move(aOperation)),
+        mFilename(std::move(aFilename)),
+        mIOThreadId(aIOThreadId) {}
+
+  const char* mSource;
+  mozilla::UniqueFreePtr<char> mOperation;
+  mozilla::UniqueFreePtr<char> mFilename;
+  mozilla::Maybe<int> mIOThreadId;
+};
+
 class PrefMarkerPayload : public ProfilerMarkerPayload {
  public:
   PrefMarkerPayload(const char* aPrefName,

tools/profiler/public/ProfilerMarkerTypes.h

@@ -37,6 +37,7 @@ namespace geckoprofiler::markers {
 
 // Import some common markers from mozilla::baseprofiler::markers.
 using Tracing = mozilla::baseprofiler::markers::Tracing;
+using FileIO = mozilla::baseprofiler::markers::FileIO;
 using UserTimingMark = mozilla::baseprofiler::markers::UserTimingMark;
 using UserTimingMeasure = mozilla::baseprofiler::markers::UserTimingMeasure;
 using Hang = mozilla::baseprofiler::markers::Hang;

tools/profiler/tests/gtest/GeckoProfiler.cpp

@@ -11,7 +11,6 @@
 
 #include "GeckoProfiler.h"
 #include "mozilla/ProfilerMarkerTypes.h"
-#include "mozilla/ProfilerMarkers.h"
 #include "platform.h"
 #include "ProfileBuffer.h"
 #include "ProfilerMarkerPayload.h"
@@ -756,10 +755,23 @@ TEST(GeckoProfiler, Markers)
 
   // Keep this one first! (It's used to record `ts1` and `ts2`, to compare
   // to serialized numbers in other markers.)
-  MOZ_RELEASE_ASSERT(
+  PROFILER_ADD_MARKER_WITH_PAYLOAD(
-      profiler_add_marker("FirstMarker", geckoprofiler::category::OTHER,
+      "FileIOMarkerPayload marker", OTHER, FileIOMarkerPayload,
-                          MarkerTiming::Interval(ts1, ts2),
+      ("operation", "source", "filename", ts1, ts2, nullptr));
-                          geckoprofiler::markers::Text{}, "FirstMarker"));
+
+  MOZ_RELEASE_ASSERT(profiler_add_marker(
+      "FileIOMarkerPayload marker 2.0", geckoprofiler::category::OTHER,
+      MarkerTiming::Interval(ts1, ts2), geckoprofiler::markers::FileIO{},
+      "operation", "source", "filename", MarkerThreadId{}));
+
+  PROFILER_ADD_MARKER_WITH_PAYLOAD(
+      "FileIOMarkerPayload marker off-MT", OTHER, FileIOMarkerPayload,
+      ("operation2", "source2", "filename2", ts1, ts2, nullptr, Some(123)));
+
+  MOZ_RELEASE_ASSERT(profiler_add_marker(
+      "FileIOMarkerPayload marker 2.0 off-MT", geckoprofiler::category::OTHER,
+      MarkerTiming::Interval(ts1, ts2), geckoprofiler::markers::FileIO{},
+      "operation2", "source2", "filename2", MarkerThreadId{123}));
 
   // Other markers in alphabetical order of payload class names.
 
@@ -944,7 +956,10 @@ TEST(GeckoProfiler, Markers)
     S_Markers2DefaultWithOptions,
     S_Markers2ExplicitDefaultEmptyOptions,
     S_Markers2ExplicitDefaultWithOptions,
-    S_FirstMarker,
+    S_FileIOMarkerPayload,
+    S_FileIOMarker2,
+    S_FileIOMarkerPayloadOffMT,
+    S_FileIOMarker2OffMT,
     S_GCMajorMarkerPayload,
     S_GCMinorMarkerPayload,
     S_GCSliceMarkerPayload,
@@ -967,7 +982,7 @@ TEST(GeckoProfiler, Markers)
     S_LAST,
   } state = State(0);
 
-  // These will be set when first read from S_FirstMarker, then
+  // These will be set when first read from S_FileIOMarkerPayload, then
   // compared in following markers.
   // TODO: Compute these values from the timestamps.
   double ts1Double = 0.0;
@@ -1290,13 +1305,61 @@ TEST(GeckoProfiler, Markers)
                 EXPECT_EQ(typeString, "NoPayloadUserData");
                 EXPECT_FALSE(payload["stack"].isNull());
 
-              } else if (nameString == "FirstMarker") {
+              } else if (nameString == "FileIOMarkerPayload marker") {
+                EXPECT_EQ(state, S_FileIOMarkerPayload);
+                state = State(S_FileIOMarkerPayload + 1);
+                EXPECT_EQ(typeString, "FileIO");
+
                 // Record start and end times, to compare with timestamps in
                 // following markers.
-                EXPECT_EQ(state, S_FirstMarker);
+                EXPECT_EQ(ts1Double, 0.0);
                 ts1Double = marker[START_TIME].asDouble();
+                EXPECT_NE(ts1Double, 0.0);
+                EXPECT_EQ(ts2Double, 0.0);
                 ts2Double = marker[END_TIME].asDouble();
-                state = State(S_FirstMarker + 1);
+                EXPECT_NE(ts2Double, 0.0);
+                EXPECT_EQ_JSON(marker[PHASE], UInt, PHASE_INTERVAL);
+
+                EXPECT_TRUE(payload["stack"].isNull());
+                EXPECT_EQ_JSON(payload["operation"], String, "operation");
+                EXPECT_EQ_JSON(payload["source"], String, "source");
+                EXPECT_EQ_JSON(payload["filename"], String, "filename");
+                EXPECT_FALSE(payload.isMember("threadId"));
+
+              } else if (nameString == "FileIOMarkerPayload marker 2.0") {
+                EXPECT_EQ(state, S_FileIOMarker2);
+                state = State(S_FileIOMarker2 + 1);
+                EXPECT_EQ(typeString, "FileIO");
+                EXPECT_TIMING_INTERVAL_AT(ts1Double, ts2Double);
+                EXPECT_TRUE(payload["stack"].isNull());
+                EXPECT_EQ_JSON(payload["operation"], String, "operation");
+                EXPECT_EQ_JSON(payload["source"], String, "source");
+                EXPECT_EQ_JSON(payload["filename"], String, "filename");
+                EXPECT_FALSE(payload.isMember("threadId"));
+
+              } else if (nameString == "FileIOMarkerPayload marker off-MT") {
+                EXPECT_EQ(state, S_FileIOMarkerPayloadOffMT);
+                state = State(S_FileIOMarkerPayloadOffMT + 1);
+                EXPECT_EQ(typeString, "FileIO");
+                EXPECT_TIMING_INTERVAL_AT(ts1Double, ts2Double);
+                EXPECT_TRUE(payload["stack"].isNull());
+                EXPECT_EQ_JSON(payload["operation"], String, "operation2");
+                EXPECT_EQ_JSON(payload["source"], String, "source2");
+                EXPECT_EQ_JSON(payload["filename"], String, "filename2");
+                EXPECT_EQ_JSON(payload["threadId"], Int, 123);
+
+              } else if (nameString ==
+                         "FileIOMarkerPayload marker 2.0 off-MT") {
+                EXPECT_EQ(state, S_FileIOMarker2OffMT);
+                state = State(S_FileIOMarker2OffMT + 1);
+                EXPECT_EQ(typeString, "FileIO");
+                EXPECT_TIMING_INTERVAL_AT(ts1Double, ts2Double);
+                EXPECT_TRUE(payload["stack"].isNull());
+                EXPECT_EQ_JSON(payload["operation"], String, "operation2");
+                EXPECT_EQ_JSON(payload["source"], String, "source2");
+                EXPECT_EQ_JSON(payload["filename"], String, "filename2");
+                EXPECT_EQ_JSON(payload["threadId"], Int, 123);
+
               } else if (nameString == "GCMajorMarkerPayload marker") {
                 EXPECT_EQ(state, S_GCMajorMarkerPayload);
                 state = State(S_GCMajorMarkerPayload + 1);
@@ -1547,7 +1610,30 @@ TEST(GeckoProfiler, Markers)
           ASSERT_EQ(data.size(), 0u);
 
         } else if (nameString == "FileIO") {
-          // These are defined in ProfilerIOInterposeObserver.cpp
+          EXPECT_EQ(display.size(), 3u);
+          EXPECT_EQ(display[0u].asString(), "marker-chart");
+          EXPECT_EQ(display[1u].asString(), "marker-table");
+          EXPECT_EQ(display[2u].asString(), "timeline-fileio");
+
+          ASSERT_EQ(data.size(), 3u);
+
+          ASSERT_TRUE(data[0u].isObject());
+          EXPECT_EQ_JSON(data[0u]["key"], String, "operation");
+          EXPECT_EQ_JSON(data[0u]["label"], String, "Operation");
+          EXPECT_EQ_JSON(data[0u]["format"], String, "string");
+          EXPECT_EQ_JSON(data[0u]["searchable"], Bool, true);
+
+          ASSERT_TRUE(data[1u].isObject());
+          EXPECT_EQ_JSON(data[1u]["key"], String, "source");
+          EXPECT_EQ_JSON(data[1u]["label"], String, "Source");
+          EXPECT_EQ_JSON(data[1u]["format"], String, "string");
+          EXPECT_EQ_JSON(data[1u]["searchable"], Bool, true);
+
+          ASSERT_TRUE(data[2u].isObject());
+          EXPECT_EQ_JSON(data[2u]["key"], String, "filename");
+          EXPECT_EQ_JSON(data[2u]["label"], String, "Filename");
+          EXPECT_EQ_JSON(data[2u]["format"], String, "file-path");
+          EXPECT_EQ_JSON(data[2u]["searchable"], Bool, true);
 
         } else if (nameString == "tracing") {
           EXPECT_EQ(display.size(), 3u);
@@ -1694,6 +1780,7 @@ TEST(GeckoProfiler, Markers)
 
       // Check that we've got all expected schema.
       EXPECT_TRUE(testedSchemaNames.find("Text") != testedSchemaNames.end());
+      EXPECT_TRUE(testedSchemaNames.find("FileIO") != testedSchemaNames.end());
       EXPECT_TRUE(testedSchemaNames.find("tracing") != testedSchemaNames.end());
       EXPECT_TRUE(testedSchemaNames.find("UserTimingMark") !=
                   testedSchemaNames.end());

tools/profiler/tests/shared-head.js

@@ -104,7 +104,7 @@ function getPayloadsOfType(thread, type) {
  * Applies the marker schema to create individual objects for each marker
  *
  * @param {Object} thread The thread from a profile.
- * @return {InflatedMarker[]} The markers.
+ * @return {Array} The markers.
  */
 function getInflatedMarkerData(thread) {
   const { markers, stringTable } = thread;
@@ -153,37 +153,3 @@ async function stopAndGetProfile() {
   Services.profiler.StopProfiler();
   return profile;
 }
-
-/**
- * Verifies that a marker is an interval marker.
- *
- * @param {InflatedMarker} marker
- * @returns {boolean}
- */
-function isIntervalMarker(inflatedMarker) {
-  return (
-    inflatedMarker.phase === 1 &&
-    typeof inflatedMarker.startTime === "number" &&
-    typeof inflatedMarker.endTime === "number"
-  );
-}
-
-/**
- * @param {Profile} profile
- * @returns {Thread[]}
- */
-function getThreads(profile) {
-  const threads = [];
-
-  function getThreadsRecursive(process) {
-    for (const thread of process.threads) {
-      threads.push(thread);
-    }
-    for (const subprocess of process.processes) {
-      getThreadsRecursive(subprocess);
-    }
-  }
-
-  getThreadsRecursive(profile);
-  return threads;
-}

tools/profiler/tests/xpcshell/head.js

@@ -118,61 +118,3 @@ function expectStackToContain(
 
   Assert.ok(true, message);
 }
-
-/**
- * @param {Thread} thread
- * @param {string} filename - The filename used to trigger FileIO.
- * @returns {InflatedMarkers[]}
- */
-function getInflatedFileIOMarkers(thread, filename) {
-  const markers = getInflatedMarkerData(thread);
-  return markers.filter(
-    marker =>
-      marker.data?.type === "FileIO" && marker.data?.filename.endsWith(filename)
-  );
-}
-
-/**
- * Checks properties common to all FileIO markers.
- *
- * @param {InflatedMarkers[]} markers
- * @param {string} filename
- */
-function checkInflatedFileIOMarkers(markers, filename) {
-  greater(markers.length, 0, "Found some markers");
-
-  // See IOInterposeObserver::Observation::ObservedOperationString
-  const validOperations = new Set([
-    "write",
-    "fsync",
-    "close",
-    "stat",
-    "create/open",
-    "read",
-  ]);
-  const validSources = new Set(["PoisonIOInterposer", "NSPRIOInterposer"]);
-
-  for (const marker of markers) {
-    try {
-      ok(
-        marker.name.startsWith("FileIO"),
-        "Has a marker.name that starts with FileIO"
-      );
-      equal(marker.data.type, "FileIO", "Has a marker.data.type");
-      ok(isIntervalMarker(marker), "All FileIO markers are interval markers");
-      ok(
-        validOperations.has(marker.data.operation),
-        `The markers have a known operation - "${marker.data.operation}"`
-      );
-      ok(
-        validSources.has(marker.data.source),
-        `The FileIO marker has a known source "${marker.data.source}"`
-      );
-      ok(marker.data.filename.endsWith(filename));
-      ok(Boolean(marker.data.stack), "A stack was collected");
-    } catch (error) {
-      console.error("Failing inflated FileIO marker:", marker);
-      throw error;
-    }
-  }
-}

tools/profiler/tests/xpcshell/test_feature_fileioall.js

@@ -1,171 +0,0 @@
-/* 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/. */
-
-// This is only needed for getting a temp file location, which IOUtils
-// cannot currently do.
-const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm");
-
-add_task(async () => {
-  if (!AppConstants.MOZ_GECKO_PROFILER) {
-    return;
-  }
-  info(
-    "Test that off-main thread fileio is captured for a profiled thread, " +
-      "and that it will be sent to the main thread."
-  );
-  const filename = "test_marker_fileio";
-  const profile = await startProfilerAndTriggerFileIO({
-    features: ["fileioall"],
-    threadsFilter: ["GeckoMain", "BackgroundThreadPool"],
-    filename,
-  });
-
-  const threads = getThreads(profile);
-  const mainThread = threads.find(thread => thread.name === "GeckoMain");
-  const mainThreadFileIO = getInflatedFileIOMarkers(mainThread, filename);
-  let backgroundThread;
-  let backgroundThreadFileIO;
-  for (const thread of threads) {
-    // Check for FileIO in any of the background threads.
-    if (thread.name.startsWith("BackgroundThreadPool")) {
-      const markers = getInflatedFileIOMarkers(thread, filename);
-      if (markers.length > 0) {
-        backgroundThread = thread;
-        backgroundThreadFileIO = markers;
-        break;
-      }
-    }
-  }
-
-  info("Check all of the main thread FileIO markers.");
-  checkInflatedFileIOMarkers(mainThreadFileIO, filename);
-  for (const { data, name } of mainThreadFileIO) {
-    equal(
-      name,
-      "FileIO (non-main thread)",
-      "The markers from off main thread are labeled as such."
-    );
-    equal(
-      data.threadId,
-      backgroundThread.tid,
-      "The main thread FileIO markers were all sent from the background thread."
-    );
-  }
-
-  info("Check all of the background thread FileIO markers.");
-  checkInflatedFileIOMarkers(backgroundThreadFileIO, filename);
-  for (const { data, name } of backgroundThreadFileIO) {
-    equal(
-      name,
-      "FileIO",
-      "The markers on the thread where they were generated just say FileIO"
-    );
-    equal(
-      data.threadId,
-      undefined,
-      "The background thread FileIO correctly excludes the threadId."
-    );
-  }
-});
-
-add_task(async () => {
-  if (!AppConstants.MOZ_GECKO_PROFILER) {
-    return;
-  }
-  info(
-    "Test that off-main thread fileio is captured for a thread that is not profiled, " +
-      "and that it will be sent to the main thread."
-  );
-  const filename = "test_marker_fileio";
-  const profile = await startProfilerAndTriggerFileIO({
-    features: ["fileioall"],
-    threadsFilter: ["GeckoMain"],
-    filename,
-  });
-
-  const threads = getThreads(profile);
-  const mainThread = threads.find(thread => thread.name === "GeckoMain");
-  const mainThreadFileIO = getInflatedFileIOMarkers(mainThread, filename);
-
-  info("Check all of the main thread FileIO markers.");
-  checkInflatedFileIOMarkers(mainThreadFileIO, filename);
-  for (const { data, name } of mainThreadFileIO) {
-    equal(
-      name,
-      "FileIO (non-profiled thread)",
-      "The markers from off main thread are labeled as such."
-    );
-    equal(typeof data.threadId, "number", "A thread ID is captured.");
-  }
-});
-
-/**
- * @typedef {Object} TestConfig
- * @prop {Array} features The list of profiler features
- * @prop {string[]} threadsFilter The list of threads to profile
- * @prop {string} filename A filename to trigger a write operation
- */
-
-/**
- * Start the profiler and get FileIO markers.
- * @param {TestConfig}
- * @returns {Profile}
- */
-async function startProfilerAndTriggerFileIO({
-  features,
-  threadsFilter,
-  filename,
-}) {
-  const entries = 10000;
-  const interval = 10;
-  Services.profiler.StartProfiler(entries, interval, features, threadsFilter);
-
-  const tmpDir = OS.Constants.Path.tmpDir;
-  const path = OS.Path.join(tmpDir, filename);
-
-  info(`Using a temporary file to test FileIO: ${path}`);
-
-  if (fileExists(path)) {
-    console.warn(
-      "This test is triggering FileIO by writing to a file. However, the test found an " +
-        "existing file at the location it was trying to write to. This could happen " +
-        "because a previous run of the test failed to clean up after itself. This test " +
-        " will now clean up that file before running the test again."
-    );
-    await removeFile(path);
-  }
-
-  info("Write to the file, but do so using a background thread.");
-
-  // IOUtils handles file operations using a background thread.
-  await IOUtils.writeAtomic(path, new TextEncoder().encode("Test data."));
-  const exists = await fileExists(path);
-  ok(exists, `Created temporary file at: ${path}`);
-
-  info("Remove the file");
-  await removeFile(path);
-
-  // Pause the profiler as we don't need to collect more samples as we retrieve
-  // and serialize the profile.
-  Services.profiler.Pause();
-
-  const profile = await Services.profiler.getProfileDataAsync();
-  Services.profiler.StopProfiler();
-  return profile;
-}
-
-async function fileExists(file) {
-  try {
-    let { type } = await IOUtils.stat(file);
-    return type === "regular";
-  } catch (_error) {
-    return false;
-  }
-}
-
-async function removeFile(file) {
-  await IOUtils.remove(file);
-  const exists = await fileExists(file);
-  ok(!exists, `Removed temporary file: ${file}`);
-}

tools/profiler/tests/xpcshell/test_feature_mainthreadio.js

@@ -17,21 +17,35 @@ add_task(async () => {
     return;
   }
 
+  // Let the test harness settle, in order to avoid extraneous FileIO operations. This
+  // helps avoid false positives that we are actually triggering FileIO.
+  await wait(10);
+
   {
     const filename = "profiler-mainthreadio-test-firstrun";
-    const markers = await startProfilerAndGetFileIOPayloads(
+    const payloads = await startProfilerAndgetFileIOPayloads(
       ["mainthreadio"],
       filename
     );
-    info("Check the FileIO markers when using the mainthreadio feature");
+
-    checkInflatedFileIOMarkers(markers, filename);
+    greater(
+      payloads.length,
+      0,
+      "FileIO markers were found when using the mainthreadio feature on the profiler."
+    );
+
+    // It would be better to check on the filename, but Linux does not currently include
+    // it. See https://bugzilla.mozilla.org/show_bug.cgi?id=1533531
+    // ok(hasWritePayload(payloads, filename),
+    //    "A FileIO marker is found when using the mainthreadio feature on the profiler.");
   }
 
   {
     const filename = "profiler-mainthreadio-test-no-instrumentation";
-    const markers = await startProfilerAndGetFileIOPayloads([], filename);
+    const payloads = await startProfilerAndgetFileIOPayloads([], filename);
+
     equal(
-      markers.length,
+      payloads.length,
       0,
       "No FileIO markers are found when the mainthreadio feature is not turned on " +
         "in the profiler."
@@ -40,28 +54,36 @@ add_task(async () => {
 
   {
     const filename = "profiler-mainthreadio-test-secondrun";
-    const markers = await startProfilerAndGetFileIOPayloads(
+    const payloads = await startProfilerAndgetFileIOPayloads(
       ["mainthreadio"],
       filename
     );
-    info("Check the FileIO markers when re-starting the mainthreadio feature");
+
-    checkInflatedFileIOMarkers(markers, filename);
+    greater(
+      payloads.length,
+      0,
+      "FileIO markers were found when re-starting the mainthreadio feature on the " +
+        "profiler."
+    );
+    // It would be better to check on the filename, but Linux does not currently include
+    // it. See https://bugzilla.mozilla.org/show_bug.cgi?id=1533531
+    // ok(hasWritePayload(payloads, filename),
+    //    "Re-enabling the mainthreadio re-installs the interposer, and we can capture " +
+    //    "another FileIO payload.");
   }
 });
 
 /**
- * Start the profiler and get FileIO markers.
+ * Start the profiler and get FileIO payloads.
  * @param {Array} features The list of profiler features
  * @param {string} filename A filename to trigger a write operation
- * @returns {InflatedMarkers[]}
  */
-async function startProfilerAndGetFileIOPayloads(features, filename) {
+async function startProfilerAndgetFileIOPayloads(features, filename) {
   const entries = 10000;
   const interval = 10;
   const threads = [];
   Services.profiler.StartProfiler(entries, interval, features, threads);
 
-  info("Get the file");
   const file = FileUtils.getFile("TmpD", [filename]);
   if (file.exists()) {
     console.warn(
@@ -73,28 +95,33 @@ async function startProfilerAndGetFileIOPayloads(features, filename) {
     file.remove(false);
   }
 
-  info(
-    "Generate file IO on the main thread using FileUtils.openSafeFileOutputStream."
-  );
   const outputStream = FileUtils.openSafeFileOutputStream(file);
 
   const data = "Test data.";
-  info("Write to the file");
   outputStream.write(data, data.length);
-
-  info("Close the file");
   FileUtils.closeSafeFileOutputStream(outputStream);
 
-  info("Remove the file");
   file.remove(false);
 
+  // Wait for the profiler to collect a sample.
+  // See https://bugzilla.mozilla.org/show_bug.cgi?id=1529053
+  await wait(500);
+
   // Pause the profiler as we don't need to collect more samples as we retrieve
   // and serialize the profile.
   Services.profiler.Pause();
 
   const profile = await Services.profiler.getProfileDataAsync();
   Services.profiler.StopProfiler();
-  const mainThread = profile.threads.find(({ name }) => name === "GeckoMain");
+  return getPayloadsOfTypeFromAllThreads(profile, "FileIO");
-
+}
-  return getInflatedFileIOMarkers(mainThread, filename);
+
+/**
+ * See if a list of payloads has a write operation from a file.
+ *
+ * @param {Array<Object>} payloads The payloads captured from the profiler.
+ * @param {string} filename The filename used to test a write operation.
+ */
+function hasWritePayload(payloads, filename) {
+  return payloads.some(payload => payload.filename.endsWith(filename));
 }

tools/profiler/tests/xpcshell/xpcshell.ini

@@ -23,8 +23,6 @@ skip-if = !debug
 [test_asm.js]
 [test_feature_mainthreadio.js]
 skip-if = release_or_beta || (os == "win" && processor == "aarch64") # The IOInterposer is in an ifdef, aarch64 due to 1536657
-[test_feature_fileioall.js]
-skip-if = release_or_beta || (os == "win" && processor == "aarch64") # The IOInterposer is in an ifdef, aarch64 due to 1536657
 
 # The sanitizer checks appears to overwrite our own memory hooks in xpcshell tests,
 # and no allocation markers are gathered. Skip this test in that configuration.