Bug 1278821 - Re-factor JS-exposed functions in TelemetryHistogram.cpp to avoid deadlocks. r=Dexter
authorfernando <mortificador@gmail.com>
Thu, 11 Jan 2018 22:01:15 +0000
changeset 450952 b8ded26d5d1960d2cb3cf8e46409643d811ba333
parent 450951 071076af815da074a65c6f8a09ffc02a6a67fa95
child 450953 8be5c5375a9010dbf91d415ccf5950a3c7c1a2c0
push id8543
push userryanvm@gmail.com
push dateTue, 16 Jan 2018 14:33:22 +0000
treeherdermozilla-beta@a6525ed16a32 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter
bugs1278821
milestone59.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 1278821 - Re-factor JS-exposed functions in TelemetryHistogram.cpp to avoid deadlocks. r=Dexter
toolkit/components/telemetry/TelemetryHistogram.cpp
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -1138,27 +1138,23 @@ internal_JSHistogram_Add(JSContext *cx, 
   MOZ_ASSERT(internal_IsHistogramEnumId(id));
   uint32_t type = gHistogramInfos[id].histogramType;
 
   JS::CallArgs args = CallArgsFromVp(argc, vp);
   // This function should always return |undefined| and never fail but
   // rather report failures using the console.
   args.rval().setUndefined();
 
-  if (!internal_CanRecordBase()) {
-    return true;
-  }
-
   uint32_t value = 0;
   if ((type == nsITelemetry::HISTOGRAM_COUNT) && (args.length() == 0)) {
     // If we don't have an argument for the count histogram, assume an increment of 1.
     // Otherwise, make sure to run some sanity checks on the argument.
     value = 1;
   } else if ((args.length() > 0) && args[0].isString() &&
-             gHistogramInfos[id].histogramType == nsITelemetry::HISTOGRAM_CATEGORICAL) {
+             type == nsITelemetry::HISTOGRAM_CATEGORICAL) {
     // For categorical histograms we allow passing a string argument that specifies the label.
     nsAutoJSString label;
     if (!label.init(cx, args[0])) {
       LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
       return true;
     }
 
     // Get label id value.
@@ -1203,30 +1199,41 @@ internal_JSHistogram_Snapshot(JSContext 
       JS_GetClass(obj) != &sJSHistogramClass) {
     JS_ReportErrorASCII(cx, "Wrong JS class, expected JSHistogram class");
     return false;
   }
 
   JSHistogramData* data = static_cast<JSHistogramData*>(JS_GetPrivate(obj));
   MOZ_ASSERT(data);
   HistogramID id = data->histogramId;
-  MOZ_ASSERT(internal_IsHistogramEnumId(id));
+
+  Histogram* h = nullptr;
+  Histogram::SampleSet ss;
+  {
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+    MOZ_ASSERT(internal_IsHistogramEnumId(id));
 
-  // This is not good standard behavior given that we have histogram instances
-  // covering multiple processes and two session types.
-  // However, changing this requires some broader changes to callers.
-  Histogram* h = internal_GetHistogramById(id, ProcessID::Parent, SessionType::Session);
-  MOZ_ASSERT(h);
+    // This is not good standard behavior given that we have histogram instances
+    // covering multiple processes and two session types.
+    // However, changing this requires some broader changes to callers.
+    h = internal_GetHistogramById(id, ProcessID::Parent, SessionType::Session);
+    // Take a snapshot of Histogram::SampleSet here, protected by the lock,
+    // and then, outside of the lock protection, mirror it to a JS structure
+    MOZ_ASSERT(h);
+    h->SnapshotSample(&ss);
+  }
 
   JS::Rooted<JSObject*> snapshot(cx, JS_NewPlainObject(cx));
   if (!snapshot) {
     return false;
   }
 
-  switch (internal_ReflectHistogramSnapshot(cx, snapshot, h)) {
+  reflectStatus status = internal_ReflectHistogramAndSamples(cx, snapshot, h, ss);
+
+  switch (status) {
   case REFLECT_FAILURE:
     return false;
   case REFLECT_OK:
     args.rval().setObject(*snapshot);
     return true;
   default:
     MOZ_ASSERT_UNREACHABLE("Unhandled reflection status.");
   }
@@ -1241,18 +1248,16 @@ internal_JSHistogram_Clear(JSContext *cx
   if (!obj ||
       JS_GetClass(obj) != &sJSHistogramClass) {
     JS_ReportErrorASCII(cx, "Wrong JS class, expected JSHistogram class");
     return false;
   }
 
   JSHistogramData* data = static_cast<JSHistogramData*>(JS_GetPrivate(obj));
   MOZ_ASSERT(data);
-  HistogramID id = data->histogramId;
-  MOZ_ASSERT(internal_IsHistogramEnumId(id));
 
   bool onlySubsession = false;
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
   // This function should always return |undefined| and never fail but
   // rather report failures using the console.
   args.rval().setUndefined();
 
 #if !defined(MOZ_WIDGET_ANDROID)
@@ -1261,17 +1266,23 @@ internal_JSHistogram_Clear(JSContext *cx
       JS_ReportErrorASCII(cx, "Not a boolean");
       return false;
     }
 
     onlySubsession = JS::ToBoolean(args[0]);
   }
 #endif
 
-  internal_ClearHistogram(id, onlySubsession);
+  HistogramID id = data->histogramId;
+  {
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+
+    MOZ_ASSERT(internal_IsHistogramEnumId(id));
+    internal_ClearHistogram(id, onlySubsession);
+  }
 
   return true;
 }
 
 // NOTE: Runs without protection from |gTelemetryHistogramMutex|.
 // See comment at the top of this section.
 nsresult
 internal_WrapAndReturnHistogram(HistogramID id, JSContext *cx,
@@ -1513,17 +1524,20 @@ internal_JSKeyedHistogram_Add(JSContext 
 
       if (!JS::ToUint32(cx, args[1], &value)) {
         LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
         return true;
       }
     }
   }
 
-  internal_Accumulate(id, NS_ConvertUTF16toUTF8(key), value);
+  {
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+    internal_Accumulate(id, NS_ConvertUTF16toUTF8(key), value);
+  }
 
   return true;
 }
 
 bool
 internal_JSKeyedHistogram_Keys(JSContext *cx, unsigned argc, JS::Value *vp)
 {
   JSObject *obj = JS_THIS_OBJECT(cx, vp);
@@ -1531,22 +1545,28 @@ internal_JSKeyedHistogram_Keys(JSContext
       JS_GetClass(obj) != &sJSKeyedHistogramClass) {
     JS_ReportErrorASCII(cx, "Wrong JS class, expected JSKeyedHistogram class");
     return false;
   }
 
   JSHistogramData* data = static_cast<JSHistogramData*>(JS_GetPrivate(obj));
   MOZ_ASSERT(data);
   HistogramID id = data->histogramId;
-  MOZ_ASSERT(internal_IsHistogramEnumId(id));
+
+  KeyedHistogram* keyed = nullptr;
+  {
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+    MOZ_ASSERT(internal_IsHistogramEnumId(id));
 
-  // This is not good standard behavior given that we have histogram instances
-  // covering multiple processes and two session types.
-  // However, changing this requires some broader changes to callers.
-  KeyedHistogram* keyed = internal_GetKeyedHistogramById(id, ProcessID::Parent);
+    // This is not good standard behavior given that we have histogram instances
+    // covering multiple processes and two session types.
+    // However, changing this requires some broader changes to callers.
+    keyed = internal_GetKeyedHistogramById(id, ProcessID::Parent);
+  }
+
   MOZ_ASSERT(keyed);
   if (!keyed) {
     return false;
   }
 
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
   return NS_SUCCEEDED(keyed->GetJSKeys(cx, args));
 }
@@ -1589,48 +1609,50 @@ internal_JSKeyedHistogram_Clear(JSContex
       JS_GetClass(obj) != &sJSKeyedHistogramClass) {
     JS_ReportErrorASCII(cx, "Wrong JS class, expected JSKeyedHistogram class");
     return false;
   }
 
   JSHistogramData* data = static_cast<JSHistogramData*>(JS_GetPrivate(obj));
   MOZ_ASSERT(data);
   HistogramID id = data->histogramId;
-  MOZ_ASSERT(internal_IsHistogramEnumId(id));
 
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
   // This function should always return |undefined| and never fail but
   // rather report failures using the console.
   args.rval().setUndefined();
 
-  // This is not good standard behavior given that we have histogram instances
-  // covering multiple processes and two session types.
-  // However, changing this requires some broader changes to callers.
-  KeyedHistogram* keyed = internal_GetKeyedHistogramById(id, ProcessID::Parent, /* instantiate = */ false);
-  if (!keyed) {
-    return true;
-  }
+  bool onlySubsession = false;
+  #if !defined(MOZ_WIDGET_ANDROID)
+    if (args.length() >= 1) {
+      if (!(args[0].isNumber() || args[0].isBoolean())) {
+        JS_ReportErrorASCII(cx, "Not a boolean");
+        return false;
+      }
+      onlySubsession = JS::ToBoolean(args[0]);
+    }
+  #endif
 
-#if !defined(MOZ_WIDGET_ANDROID)
-  bool onlySubsession = false;
+  KeyedHistogram* keyed = nullptr;
+  {
+    MOZ_ASSERT(internal_IsHistogramEnumId(id));
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
 
-  if (args.length() >= 1) {
-    if (!(args[0].isNumber() || args[0].isBoolean())) {
-      JS_ReportErrorASCII(cx, "Not a boolean");
-      return false;
+    // This is not good standard behavior given that we have histogram instances
+    // covering multiple processes and two session types.
+    // However, changing this requires some broader changes to callers.
+    keyed = internal_GetKeyedHistogramById(id, ProcessID::Parent, /* instantiate = */ false);
+
+    if (!keyed) {
+      return true;
     }
 
-    onlySubsession = JS::ToBoolean(args[0]);
+    keyed->Clear(onlySubsession);
   }
 
-  keyed->Clear(onlySubsession);
-#else
-  keyed->Clear(false);
-#endif
-
   return true;
 }
 
 // NOTE: Runs without protection from |gTelemetryHistogramMutex|.
 // See comment at the top of this section.
 nsresult
 internal_WrapAndReturnKeyedHistogram(HistogramID id, JSContext *cx,
                                      JS::MutableHandle<JS::Value> ret)
@@ -2132,71 +2154,112 @@ TelemetryHistogram::CreateHistogramSnaps
   // to launch a process for it.
   bool includeGPUProcess = false;
   if (auto gpm = mozilla::gfx::GPUProcessManager::Get()) {
     includeGPUProcess = gpm->AttemptedGPUProcess();
   }
 
   SessionType sessionType = SessionType(aSubsession);
 
-  for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
+  // Struct used to keep information about the histograms for which a
+  // snapshot should be created
+  struct MOZ_NON_MEMMOVABLE HistogramProcessInfo {
+    Histogram* h;
+    Histogram::SampleSet ss;
+    size_t index;
+  };
+
+  mozilla::Vector<mozilla::Vector<HistogramProcessInfo>>
+    processHistArray;
+  {
+    if (!processHistArray.resize(static_cast<uint32_t>(ProcessID::Count))) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+    for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
+      mozilla::Vector<HistogramProcessInfo>& hArray = processHistArray[process];
+
+      for (size_t i = 0; i < HistogramCount; ++i) {
+        const HistogramInfo& info = gHistogramInfos[i];
+        if (info.keyed) {
+          continue;
+        }
+
+        HistogramID id = HistogramID(i);
+
+        if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
+            ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
+          continue;
+        }
+
+        if (!IsInDataset(info.dataset, aDataset)) {
+          continue;
+        }
+
+        bool shouldInstantiate =
+          info.histogramType == nsITelemetry::HISTOGRAM_FLAG;
+        Histogram* h = internal_GetHistogramById(id, ProcessID(process),
+                                                 sessionType,
+                                                 shouldInstantiate);
+        if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
+          continue;
+        }
+
+        Histogram::SampleSet ss;
+        h->SnapshotSample(&ss);
+        if (!hArray.emplaceBack(HistogramProcessInfo{h, ss, i})) {
+          return NS_ERROR_OUT_OF_MEMORY;
+        }
+
+#if !defined(MOZ_WIDGET_ANDROID)
+        if ((sessionType == SessionType::Subsession) && aClearSubsession) {
+          h->Clear();
+        }
+#endif
+      }
+    }
+  }
+
+  // Make the JS calls on the stashed histograms for every process
+  for (uint32_t process = 0; process < processHistArray.length(); ++process) {
     JS::Rooted<JSObject*> processObject(aCx, JS_NewPlainObject(aCx));
     if (!processObject) {
       return NS_ERROR_FAILURE;
     }
     if (!JS_DefineProperty(aCx, root_obj,
                            GetNameForProcessID(ProcessID(process)),
                            processObject, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
-    for (size_t i = 0; i < HistogramCount; ++i) {
-      const HistogramInfo& info = gHistogramInfos[i];
-      if (info.keyed) {
-        continue;
-      }
-
-      HistogramID id = HistogramID(i);
-
-      if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
-        ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
-        continue;
-      }
 
-      if (!IsInDataset(info.dataset, aDataset)) {
-        continue;
-      }
+    const mozilla::Vector<HistogramProcessInfo>& hArray = processHistArray[process];
+    for (size_t i = 0; i < hArray.length(); ++i) {
+      const HistogramProcessInfo& hData = hArray[i];
+      uint32_t histogramIndex = hData.index;
 
-      bool shouldInstantiate =
-        info.histogramType == nsITelemetry::HISTOGRAM_FLAG;
-      Histogram* h = internal_GetHistogramById(id, ProcessID(process),
-                                               sessionType,
-                                               shouldInstantiate);
-      if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
-        continue;
-      }
+      HistogramID id = HistogramID(histogramIndex);
 
       JS::Rooted<JSObject*> hobj(aCx, JS_NewPlainObject(aCx));
       if (!hobj) {
         return NS_ERROR_FAILURE;
       }
-      switch (internal_ReflectHistogramSnapshot(aCx, hobj, h)) {
+
+      Histogram* h = hData.h;
+      reflectStatus status =  internal_ReflectHistogramAndSamples(aCx, hobj, h,
+                                                                  hData.ss);
+      switch (status) {
       case REFLECT_FAILURE:
         return NS_ERROR_FAILURE;
       case REFLECT_OK:
         if (!JS_DefineProperty(aCx, processObject, gHistogramInfos[id].name(),
                                hobj, JSPROP_ENUMERATE)) {
           return NS_ERROR_FAILURE;
         }
       }
-
-#if !defined(MOZ_WIDGET_ANDROID)
-      if ((sessionType == SessionType::Subsession) && aClearSubsession) {
-        h->Clear();
-      }
-#endif
     }
   }
   return NS_OK;
 }
 
 nsresult
 TelemetryHistogram::GetKeyedHistogramSnapshots(JSContext* aCx,
                                                JS::MutableHandleValue aResult,