Bug 1428076 - Fix bad JSON format when profiling workers that do nothing r=mstange draft
authorJulien Wajsberg <felash@gmail.com>
Mon, 08 Jan 2018 18:25:36 +0100
changeset 724090 72dcdcf152762be0238fc0cb66ce06ee8446cf61
parent 723473 e2bb11b88bd45bdb2e055042e1624b74d414e73c
child 724091 9829e3bcf2c7661a6dd28ac168f7b8dc1425688d
child 724177 93d2052a81d0b3cb7abc0fdc27b3a325f67886ac
push id96639
push userbmo:felash@gmail.com
push dateWed, 24 Jan 2018 13:46:30 +0000
reviewersmstange
bugs1428076
milestone60.0a1
Bug 1428076 - Fix bad JSON format when profiling workers that do nothing r=mstange When the Gecko Profiler runs, we keep samples and markers for threads in some occasions (eg when a Worker ends). But we fail to account for the case where these threads have output no sample and no marker yet. MozReview-Commit-ID: 2IEghD0v5Qd
tools/profiler/core/ProfileBuffer.h
tools/profiler/core/ProfileBufferEntry.cpp
tools/profiler/core/ThreadInfo.cpp
--- a/tools/profiler/core/ProfileBuffer.h
+++ b/tools/profiler/core/ProfileBuffer.h
@@ -44,21 +44,21 @@ public:
 
   void CollectCodeLocation(
     const char* aLabel, const char* aStr, int aLineNumber,
     const mozilla::Maybe<js::ProfileEntry::Category>& aCategory);
 
   // Maximum size of a frameKey string that we'll handle.
   static const size_t kMaxFrameKeyLength = 512;
 
-  void StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
+  bool StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            double aSinceTime, double* aOutFirstSampleTime,
                            JSContext* cx,
                            UniqueStacks& aUniqueStacks) const;
-  void StreamMarkersToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
+  bool StreamMarkersToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            const mozilla::TimeStamp& aProcessStartTime,
                            double aSinceTime,
                            UniqueStacks& aUniqueStacks) const;
   void StreamPausedRangesToJSON(SpliceableJSONWriter& aWriter,
                                 double aSinceTime) const;
 
   // Find (via |aLS|) the most recent sample for the thread denoted by
   // |aThreadId| and clone it, patching in |aProcessStartTime| as appropriate.
--- a/tools/profiler/core/ProfileBufferEntry.cpp
+++ b/tools/profiler/core/ProfileBufferEntry.cpp
@@ -695,17 +695,18 @@ private:
 //     DynamicStringFragment("://perf-")
 //     DynamicStringFragment("html.io/")
 //     DynamicStringFragment("ac0da204")
 //     DynamicStringFragment("aaa44d75")
 //     DynamicStringFragment("a800.bun")
 //     DynamicStringFragment("dle.js:2")
 //     DynamicStringFragment("5)")
 //
-void
+// This method returns true if it wrote anything to the writer.
+bool
 ProfileBuffer::StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                                    double aSinceTime,
                                    double* aOutFirstSampleTime,
                                    JSContext* aContext,
                                    UniqueStacks& aUniqueStacks) const
 {
   UniquePtr<char[]> strbuf = MakeUnique<char[]>(kMaxFrameKeyLength);
 
@@ -716,16 +717,17 @@ ProfileBuffer::StreamSamplesToJSON(Splic
     { \
       fprintf(stderr, "ProfileBuffer parse error: %s", msg); \
       MOZ_ASSERT(false, msg); \
       continue; \
     }
 
   EntryGetter e(*this);
   bool seenFirstSample = false;
+  bool haveSamples = false;
 
   for (;;) {
     // This block skips entries until we find the start of the next sample.
     // This is useful in three situations.
     //
     // - The circular buffer overwrites old entries, so when we start parsing
     //   we might be in the middle of a sample, and we must skip forward to the
     //   start of the next sample.
@@ -899,46 +901,53 @@ ProfileBuffer::StreamSamplesToJSON(Splic
     }
 
     if (e.Has() && e.Get().IsUnsharedMemory()) {
       sample.mUSS = Some(e.Get().u.mDouble);
       e.Next();
     }
 
     WriteSample(aWriter, sample);
+    haveSamples = true;
   }
 
+  return haveSamples;
   #undef ERROR_AND_CONTINUE
 }
 
-void
+// This method returns true if it wrote anything to the writer.
+bool
 ProfileBuffer::StreamMarkersToJSON(SpliceableJSONWriter& aWriter,
                                    int aThreadId,
                                    const TimeStamp& aProcessStartTime,
                                    double aSinceTime,
                                    UniqueStacks& aUniqueStacks) const
 {
   EntryGetter e(*this);
+  bool haveMarkers = false;
 
   // Stream all markers whose threadId matches aThreadId. We skip other entries,
   // because we process them in StreamSamplesToJSON().
   //
   // NOTE: The ThreadId of a marker is determined by its GetThreadId() method,
   // rather than ThreadId buffer entries, as markers can be added outside of
   // samples.
   while (e.Has()) {
     if (e.Get().IsMarker()) {
       const ProfilerMarker* marker = e.Get().u.mMarker;
       if (marker->GetTime() >= aSinceTime &&
           marker->GetThreadId() == aThreadId) {
         marker->StreamJSON(aWriter, aProcessStartTime, aUniqueStacks);
+        haveMarkers = true;
       }
     }
     e.Next();
   }
+
+  return haveMarkers;
 }
 
 static void
 AddPausedRange(SpliceableJSONWriter& aWriter, const char* aReason,
                const Maybe<double>& aStartTime, const Maybe<double>& aEndTime)
 {
   aWriter.Start();
   if (aStartTime) {
--- a/tools/profiler/core/ThreadInfo.cpp
+++ b/tools/profiler/core/ThreadInfo.cpp
@@ -90,19 +90,19 @@ ThreadInfo::StreamJSON(const ProfileBuff
                             aProcessStartTime,
                             mRegisterTime, mUnregisterTime,
                             aSinceTime, &firstSampleTime,
                             mContext,
                             mSavedStreamedSamples.get(),
                             mFirstSavedStreamedSampleTime,
                             mSavedStreamedMarkers.get(),
                             *mUniqueStacks);
-    mSavedStreamedSamples.reset();
+    mSavedStreamedSamples = nullptr;
     mFirstSavedStreamedSampleTime = 0.0;
-    mSavedStreamedMarkers.reset();
+    mSavedStreamedMarkers = nullptr;
 
     aWriter.StartObjectProperty("stackTable");
     {
       {
         JSONSchemaWriter schema(aWriter);
         schema.WriteField("prefix");
         schema.WriteField("frame");
       }
@@ -253,34 +253,74 @@ ThreadInfo::FlushSamplesAndMarkers(const
   //
   // Note that the UniqueStacks instance is persisted so that the frame-index
   // mapping is stable across JS shutdown.
   mUniqueStacks.emplace(mContext);
 
   {
     SpliceableChunkedJSONWriter b;
     b.StartBareList();
+    bool haveSamples = false;
     {
-      aBuffer.StreamSamplesToJSON(b, ThreadId(), /* aSinceTime = */ 0,
-                                  &mFirstSavedStreamedSampleTime,
-                                  mContext, *mUniqueStacks);
+      if (mSavedStreamedSamples) {
+        b.Splice(mSavedStreamedSamples.get());
+        haveSamples = true;
+      }
+
+      // We deliberately use a new variable instead of writing something like
+      // `haveSamples || aBuffer.StreamSamplesToJSON(...)` because we don't want
+      // to short-circuit the call.
+      bool streamedNewSamples =
+        aBuffer.StreamSamplesToJSON(b, ThreadId(), /* aSinceTime = */ 0,
+                                    &mFirstSavedStreamedSampleTime,
+                                    mContext, *mUniqueStacks);
+      haveSamples = haveSamples || streamedNewSamples;
     }
     b.EndBareList();
-    mSavedStreamedSamples = b.WriteFunc()->CopyData();
+
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1428076
+    // If we don't have any data, keep mSavedStreamSamples set to null. That
+    // way we won't try to splice it into the JSON later on, which would
+    // result in an invalid JSON due to stray commas.
+    if (haveSamples) {
+      mSavedStreamedSamples = b.WriteFunc()->CopyData();
+    } else {
+      mSavedStreamedSamples = nullptr;
+    }
   }
 
   {
     SpliceableChunkedJSONWriter b;
     b.StartBareList();
+    bool haveMarkers = false;
     {
-      aBuffer.StreamMarkersToJSON(b, ThreadId(), aProcessStartTime,
-                                  /* aSinceTime = */ 0, *mUniqueStacks);
+      if (mSavedStreamedMarkers) {
+        b.Splice(mSavedStreamedMarkers.get());
+        haveMarkers = true;
+      }
+
+      // We deliberately use a new variable instead of writing something like
+      // `haveMarkers || aBuffer.StreamMarkersToJSON(...)` because we don't want
+      // to short-circuit the call.
+      bool streamedNewMarkers =
+        aBuffer.StreamMarkersToJSON(b, ThreadId(), aProcessStartTime,
+                                    /* aSinceTime = */ 0, *mUniqueStacks);
+      haveMarkers = haveMarkers || streamedNewMarkers;
     }
     b.EndBareList();
-    mSavedStreamedMarkers = b.WriteFunc()->CopyData();
+
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1428076
+    // If we don't have any data, keep mSavedStreamMarkers set to null. That
+    // way we won't try to splice it into the JSON later on, which would
+    // result in an invalid JSON due to stray commas.
+    if (haveMarkers) {
+      mSavedStreamedMarkers = b.WriteFunc()->CopyData();
+    } else {
+      mSavedStreamedMarkers = nullptr;
+    }
   }
 
   // Reset the buffer. Attempting to symbolicate JS samples after mContext has
   // gone away will crash.
   aBuffer.Reset();
 }
 
 size_t