Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC. r?njn draft
authorMarkus Stange <mstange@themasta.com>
Thu, 30 Mar 2017 12:41:04 -0400
changeset 554819 ea55f24e9b334324cea9f789e19a8736ece88a88
parent 554803 38894655c89e68bcd8f45d31a0d3005f2c2b53db
child 554820 0c1c00abd486c3cb5d8631592dbf902622cda205
push id52056
push userbmo:mstange@themasta.com
push dateSun, 02 Apr 2017 21:59:28 +0000
reviewersnjn
bugs1351920
milestone55.0a1
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC. r?njn When the profiler is running in privacy mode, we don't want to include dynamic strings from PROFILER_LABEL_DYNAMIC to end up in the profile. Rather than checking this every time we enter a scope marked with PROFILER_LABEL_DYNAMIC, with this patch we will push the dynamic string into the pseudo stack entry regardless, and then check the privacy mode during sampling and ignore the dynamic string as necessary. This way we can avoid taking the profiler state lock in PROFILER_LABEL_DYNAMIC and also save a branch. MozReview-Commit-ID: 5dXrtMuFJ5r
tools/profiler/core/platform.cpp
tools/profiler/public/GeckoProfiler.h
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -409,31 +409,34 @@ AddDynamicCodeLocationTag(ProfileBuffer*
     // Cast to *((void**) to pass the text data to a void*.
     aBuffer->addTag(ProfileBufferEntry::EmbeddedString(*((void**)(&text[0]))));
   }
 }
 
 static const int SAMPLER_MAX_STRING_LENGTH = 128;
 
 static void
-AddPseudoEntry(ProfileBuffer* aBuffer, volatile js::ProfileEntry& entry,
-               PseudoStack* stack, void* lastpc)
+AddPseudoEntry(PS::LockRef aLock, ProfileBuffer* aBuffer,
+               volatile js::ProfileEntry& entry, PseudoStack* stack,
+               void* lastpc)
 {
   // Pseudo-frames with the BEGIN_PSEUDO_JS flag are just annotations and
   // should not be recorded in the profile.
   if (entry.hasFlag(js::ProfileEntry::BEGIN_PSEUDO_JS)) {
     return;
   }
 
   int lineno = -1;
 
   // First entry has kind CodeLocation. Check for magic pointer bit 1 to
   // indicate copy.
   const char* sampleLabel = entry.label();
-  const char* dynamicString = entry.getDynamicString();
+  bool includeDynamicString = !gPS->FeaturePrivacy(aLock);
+  const char* dynamicString =
+    includeDynamicString ? entry.getDynamicString() : nullptr;
   char combinedStringBuffer[SAMPLER_MAX_STRING_LENGTH];
 
   if (entry.isCopyLabel() || dynamicString) {
     if (dynamicString) {
       int bytesWritten =
         SprintfLiteral(combinedStringBuffer, "%s %s", sampleLabel, dynamicString);
       if (bytesWritten > 0) {
         sampleLabel = combinedStringBuffer;
@@ -508,18 +511,18 @@ struct AutoWalkJSStack
   ~AutoWalkJSStack() {
     if (walkAllowed) {
       WALKING_JS_STACK = false;
     }
   }
 };
 
 static void
-MergeStacksIntoProfile(ProfileBuffer* aBuffer, TickSample* aSample,
-                       NativeStack& aNativeStack)
+MergeStacksIntoProfile(PS::LockRef aLock, ProfileBuffer* aBuffer,
+                       TickSample* aSample, NativeStack& aNativeStack)
 {
   NotNull<PseudoStack*> pseudoStack = aSample->mThreadInfo->Stack();
   volatile js::ProfileEntry* pseudoFrames = pseudoStack->mStack;
   uint32_t pseudoCount = pseudoStack->stackSize();
 
   // Make a copy of the JS stack into a JSFrame array. This is necessary since,
   // like the native stack, the JS stack is iterated youngest-to-oldest and we
   // need to iterate oldest-to-youngest when adding entries to aInfo.
@@ -640,17 +643,17 @@ MergeStacksIntoProfile(ProfileBuffer* aB
                                jsStackAddr != nativeStackAddr);
     MOZ_ASSERT_IF(nativeStackAddr, nativeStackAddr != pseudoStackAddr &&
                                    nativeStackAddr != jsStackAddr);
 
     // Check to see if pseudoStack frame is top-most.
     if (pseudoStackAddr > jsStackAddr && pseudoStackAddr > nativeStackAddr) {
       MOZ_ASSERT(pseudoIndex < pseudoCount);
       volatile js::ProfileEntry& pseudoFrame = pseudoFrames[pseudoIndex];
-      AddPseudoEntry(aBuffer, pseudoFrame, pseudoStack, nullptr);
+      AddPseudoEntry(aLock, aBuffer, pseudoFrame, pseudoStack, nullptr);
       pseudoIndex++;
       continue;
     }
 
     // Check to see if JS jit stack frame is top-most
     if (jsStackAddr > nativeStackAddr) {
       MOZ_ASSERT(jsIndex >= 0);
       const JS::ProfilingFrameIterator::Frame& jsFrame = jsFrames[jsIndex];
@@ -754,17 +757,17 @@ DoNativeBacktrace(PS::LockRef aLock, Pro
   // Win64 always omits frame pointers so for it we use the slower
   // MozStackWalk().
   uintptr_t thread = GetThreadHandle(aSample->mThreadInfo->GetPlatformData());
   MOZ_ASSERT(thread);
   MozStackWalk(StackWalkCallback, /* skipFrames */ 0, maxFrames, &nativeStack,
                thread, /* platformData */ nullptr);
 #endif
 
-  MergeStacksIntoProfile(aBuffer, aSample, nativeStack);
+  MergeStacksIntoProfile(aLock, aBuffer, aSample, nativeStack);
 }
 #endif
 
 #ifdef USE_EHABI_STACKWALK
 static void
 DoNativeBacktrace(PS::LockRef aLock, ProfileBuffer* aBuffer,
                   TickSample* aSample)
 {
@@ -824,17 +827,17 @@ DoNativeBacktrace(PS::LockRef aLock, Pro
   // Now unwind whatever's left (starting from either the last EnterJIT frame
   // or, if no EnterJIT was found, the original registers).
   nativeStack.count += EHABIStackWalk(*mcontext,
                                       aSample->mThreadInfo->StackTop(),
                                       sp_array + nativeStack.count,
                                       pc_array + nativeStack.count,
                                       nativeStack.size - nativeStack.count);
 
-  MergeStacksIntoProfile(aBuffer, aSample, nativeStack);
+  MergeStacksIntoProfile(aLock, aBuffer, aSample, nativeStack);
 }
 #endif
 
 #ifdef USE_LUL_STACKWALK
 static void
 DoNativeBacktrace(PS::LockRef aLock, ProfileBuffer* aBuffer,
                   TickSample* aSample)
 {
@@ -924,32 +927,32 @@ DoNativeBacktrace(PS::LockRef aLock, Pro
     reinterpret_cast<void**>(framePCs),
     reinterpret_cast<void**>(frameSPs),
     mozilla::ArrayLength(framePCs),
     0
   };
 
   nativeStack.count = framesUsed;
 
-  MergeStacksIntoProfile(aBuffer, aSample, nativeStack);
+  MergeStacksIntoProfile(aLock, aBuffer, aSample, nativeStack);
 
   // Update stats in the LUL stats object.  Unfortunately this requires
   // three global memory operations.
   lul->mStats.mContext += 1;
   lul->mStats.mCFI     += framesUsed - 1 - scannedFramesAcquired;
   lul->mStats.mScanned += scannedFramesAcquired;
 }
 #endif
 
 static void
 DoSampleStackTrace(PS::LockRef aLock, ProfileBuffer* aBuffer,
                    TickSample* aSample)
 {
   NativeStack nativeStack = { nullptr, nullptr, 0, 0 };
-  MergeStacksIntoProfile(aBuffer, aSample, nativeStack);
+  MergeStacksIntoProfile(aLock, aBuffer, aSample, nativeStack);
 
   if (gPS->FeatureLeaf(aLock)) {
     aBuffer->addTag(ProfileBufferEntry::NativeLeafAddr((void*)aSample->mPC));
   }
 }
 
 // This function is called for each sampling period with the current program
 // counter. It is called within a signal and so must be re-entrant.
@@ -2848,28 +2851,16 @@ profiler_time()
 
   PS::AutoLock lock(gPSMutex);
 
   mozilla::TimeDuration delta =
     mozilla::TimeStamp::Now() - gPS->StartTime(lock);
   return delta.ToMilliseconds();
 }
 
-bool
-profiler_is_active_and_not_in_privacy_mode()
-{
-  // This function runs both on and off the main thread.
-
-  MOZ_RELEASE_ASSERT(gPS);
-
-  PS::AutoLock lock(gPSMutex);
-
-  return gPS->IsActive(lock) && !gPS->FeaturePrivacy(lock);
-}
-
 UniqueProfilerBacktrace
 profiler_get_backtrace()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_RELEASE_ASSERT(gPS);
 
   PS::AutoLock lock(gPSMutex);
 
@@ -2922,27 +2913,35 @@ void
 profiler_get_backtrace_noalloc(char *output, size_t outputSize)
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_RELEASE_ASSERT(gPS);
 
   MOZ_ASSERT(outputSize >= 2);
   char *bound = output + outputSize - 2;
   output[0] = output[1] = '\0';
+
   PseudoStack *pseudoStack = tlsPseudoStack.get();
   if (!pseudoStack) {
     return;
   }
 
+  bool includeDynamicString = true;
+  {
+    PS::AutoLock lock(gPSMutex);
+    includeDynamicString = !gPS->FeaturePrivacy(lock);
+  }
+
   volatile js::ProfileEntry *pseudoFrames = pseudoStack->mStack;
   uint32_t pseudoCount = pseudoStack->stackSize();
 
   for (uint32_t i = 0; i < pseudoCount; i++) {
     const char* label = pseudoFrames[i].label();
-    const char* dynamicString = pseudoFrames[i].getDynamicString();
+    const char* dynamicString =
+      includeDynamicString ? pseudoFrames[i].getDynamicString() : nullptr;
     size_t labelLength = strlen(label);
     if (dynamicString) {
       // Put the label, a space, and the dynamic string into output.
       size_t dynamicStringLength = strlen(dynamicString);
       if (output + labelLength + 1 + dynamicStringLength >= bound) {
         break;
       }
       strcpy(output, label);
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -289,18 +289,16 @@ PROFILER_FUNC(bool profiler_thread_is_sl
 // on auxiliary threads. Operates the same whether the profiler is active or
 // not.
 PROFILER_FUNC_VOID(profiler_js_interrupt_callback())
 
 // Gets the time since the last profiler_init() or profiler_start() call.
 // Operates the same whether the profiler is active or inactive.
 PROFILER_FUNC(double profiler_time(), 0)
 
-PROFILER_FUNC(bool profiler_is_active_and_not_in_privacy_mode(), false)
-
 PROFILER_FUNC_VOID(profiler_log(const char *str))
 
 // End of the functions defined whether the profiler is enabled or not.
 
 #if defined(MOZ_GECKO_PROFILER)
 
 #include <stdlib.h>
 #include <signal.h>
@@ -350,27 +348,27 @@ extern ProfilerState* gPS;
 #endif
 
 // Returns a handle to pass on exit. This can check that we are popping the
 // correct callstack. Operates the same whether the profiler is active or not.
 static inline void*
 profiler_call_enter(const char* aInfo,
                     js::ProfileEntry::Category aCategory,
                     void *aFrameAddress, bool aCopy, uint32_t line,
-                    const char* aAnnotationString = nullptr)
+                    const char* aDynamicString = nullptr)
 {
   // This function runs both on and off the main thread.
 
   MOZ_RELEASE_ASSERT(gPS);
 
   PseudoStack* stack = tlsPseudoStack.get();
   if (!stack) {
     return stack;
   }
-  stack->push(aInfo, aCategory, aFrameAddress, aCopy, line, aAnnotationString);
+  stack->push(aInfo, aCategory, aFrameAddress, aCopy, line, aDynamicString);
 
   // The handle is meant to support future changes but for now it is simply
   // used to avoid having to call tlsPseudoStack.get() in profiler_call_exit().
   return stack;
 }
 
 static inline void
 profiler_call_exit(void* aHandle)
@@ -511,21 +509,17 @@ public:
   ~SamplerStackFrameDynamicRAII() {
     profiler_call_exit(mHandle);
   }
 
 private:
   void* Enter(const char* aInfo, js::ProfileEntry::Category aCategory,
               uint32_t aLine, const char* aDynamicString)
   {
-    if (profiler_is_active_and_not_in_privacy_mode()) {
-      return profiler_call_enter(aInfo, aCategory, this, true, aLine, aDynamicString);
-    } else {
-      return profiler_call_enter(aInfo, aCategory, this, false, aLine);
-    }
+    return profiler_call_enter(aInfo, aCategory, this, true, aLine, aDynamicString);
   }
 
   nsCString mDynamicStorage;
   void* mHandle;
 };
 
 } // namespace mozilla