bug 1218576 - Ensure IPCTimer is on the main thread. r=froydnj f=gfritzsche
authorChris H-C <chutten@mozilla.com>
Wed, 31 Aug 2016 13:31:30 -0400
changeset 314452 fcfa94c81e0a437817caff6497ef2ace29b92ccb
parent 314451 f3b858440d0112e121efd0080e2eaf9f081d6d36
child 314453 1b498933da3ae553f8a965c4b20e40fbe57fd1bf
push id20574
push usercbook@mozilla.com
push dateTue, 20 Sep 2016 10:05:16 +0000
treeherderfx-team@14705f779a46 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1218576, 1299312
milestone51.0a1
bug 1218576 - Ensure IPCTimer is on the main thread. r=froydnj f=gfritzsche nsTimer fires on the thread that created the timer. An nsTimer instance should only be manipulated on its target thread (it isn't threadsafe). IPC using PContent must be on the main thread. Thus, everything to do with the gIPCTimer must be on the main thread. This also takes care of bug 1299312. MozReview-Commit-ID: IcVRYsoX2R9
toolkit/components/telemetry/TelemetryHistogram.cpp
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -11,16 +11,17 @@
 #include "nsTHashtable.h"
 #include "nsHashKeys.h"
 #include "nsBaseHashtable.h"
 #include "nsClassHashtable.h"
 #include "nsITelemetry.h"
 
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ToJSValue.h"
+#include "mozilla/Atomics.h"
 #include "mozilla/StartupTimeline.h"
 #include "mozilla/StaticMutex.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/Unused.h"
 
 #include "TelemetryCommon.h"
 #include "TelemetryHistogram.h"
 
@@ -189,17 +190,18 @@ bool gCorruptHistograms[mozilla::Telemet
 
 AddonMapType gAddonMap;
 
 // The singleton StatisticsRecorder object for this process.
 base::StatisticsRecorder* gStatisticsRecorder = nullptr;
 
 // For batching and sending child process accumulations to the parent
 nsITimer* gIPCTimer = nullptr;
-bool gIPCTimerArmed = false;
+mozilla::Atomic<bool, mozilla::Relaxed> gIPCTimerArmed(false);
+mozilla::Atomic<bool, mozilla::Relaxed> gIPCTimerArming(false);
 StaticAutoPtr<nsTArray<Accumulation>> gAccumulations;
 StaticAutoPtr<nsTArray<KeyedAccumulation>> gKeyedAccumulations;
 
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
@@ -1228,16 +1230,24 @@ internal_AddonReflector(AddonEntryType *
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: thread-unsafe helpers for the external interface
 
+// This is a StaticMutex rather than a plain Mutex (1) so that
+// it gets initialised in a thread-safe manner the first time
+// it is used, and (2) because it is never de-initialised, and
+// a normal Mutex would show up as a leak in BloatView.  StaticMutex
+// also has the "OffTheBooks" property, so it won't show as a leak
+// in BloatView.
+static StaticMutex gTelemetryHistogramMutex;
+
 namespace {
 
 void
 internal_SetHistogramRecordingEnabled(mozilla::Telemetry::ID aID, bool aEnabled)
 {
   if (!internal_IsHistogramEnumId(aID)) {
     MOZ_ASSERT(false, "Telemetry::SetHistogramRecordingEnabled(...) must be used with an enum id");
     return;
@@ -1257,32 +1267,50 @@ internal_SetHistogramRecordingEnabled(mo
       h->SetRecordingEnabled(aEnabled);
       return;
     }
   }
 
   MOZ_ASSERT(false, "Telemetry::SetHistogramRecordingEnabled(...) id not found");
 }
 
-void internal_armIPCTimer()
+void internal_armIPCTimerMainThread()
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  gIPCTimerArming = false;
   if (gIPCTimerArmed) {
     return;
   }
   if (!gIPCTimer) {
     CallCreateInstance(NS_TIMER_CONTRACTID, &gIPCTimer);
   }
   if (gIPCTimer) {
     gIPCTimer->InitWithFuncCallback(TelemetryHistogram::IPCTimerFired,
                                     nullptr, kBatchTimeoutMs,
                                     nsITimer::TYPE_ONE_SHOT);
     gIPCTimerArmed = true;
   }
 }
 
+void internal_armIPCTimer()
+{
+  if (gIPCTimerArmed || gIPCTimerArming) {
+    return;
+  }
+  gIPCTimerArming = true;
+  if (NS_IsMainThread()) {
+    internal_armIPCTimerMainThread();
+  } else {
+    NS_DispatchToMainThread(NS_NewRunnableFunction([]() -> void {
+      StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+      internal_armIPCTimerMainThread();
+    }));
+  }
+}
+
 bool
 internal_RemoteAccumulate(mozilla::Telemetry::ID aId, uint32_t aSample)
 {
   if (XRE_IsParentProcess()) {
     return false;
   }
   if (!gAccumulations) {
     gAccumulations = new nsTArray<Accumulation>();
@@ -1879,24 +1907,16 @@ internal_WrapAndReturnKeyedHistogram(Key
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // EXTERNALLY VISIBLE FUNCTIONS in namespace TelemetryHistogram::
 
-// This is a StaticMutex rather than a plain Mutex (1) so that
-// it gets initialised in a thread-safe manner the first time
-// it is used, and (2) because it is never de-initialised, and
-// a normal Mutex would show up as a leak in BloatView.  StaticMutex
-// also has the "OffTheBooks" property, so it won't show as a leak
-// in BloatView.
-static StaticMutex gTelemetryHistogramMutex;
-
 // All of these functions are actually in namespace TelemetryHistogram::,
 // but the ::TelemetryHistogram prefix is given explicitly.  This is
 // because it is critical to see which calls from these functions are
 // to another function in this interface.  Mis-identifying "inwards
 // calls" from "calls to another function in this interface" will lead
 // to deadlocking and/or races.  See comments at the top of the file
 // for further (important!) details.
 
@@ -2550,21 +2570,21 @@ TelemetryHistogram::GetHistogramSizesofI
 
 // This method takes the lock only to double-buffer the batched telemetry.
 // It releases the lock before calling out to IPC code which can (and does)
 // Accumulate (which would deadlock)
 //
 // To ensure we don't loop IPCTimerFired->AccumulateChild->arm timer, we don't
 // unset gIPCTimerArmed until the IPC completes
 //
-// This function may be re-entered. The shared datastructures gAccumulations and
-// gKeyedAccumulations are guarded by the lock.
+// This function must be called on the main thread, otherwise IPC will fail.
 void
 TelemetryHistogram::IPCTimerFired(nsITimer* aTimer, void* aClosure)
 {
+  MOZ_ASSERT(NS_IsMainThread());
   nsTArray<Accumulation> accumulationsToSend;
   nsTArray<KeyedAccumulation> keyedAccumulationsToSend;
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     if (gAccumulations) {
       accumulationsToSend.SwapElements(*gAccumulations);
     }
     if (gKeyedAccumulations) {