bug 1218576 - Ensure IPCTimer is on the main thread. r=froydnj f=gfritzsche
☠☠ backed out by 1530103e032c ☠ ☠
authorChris H-C <chutten@mozilla.com>
Wed, 31 Aug 2016 13:31:30 -0400
changeset 313008 62009556e4ad9cdb7910a3b5b0346a1c4902d582
parent 313007 57f9849f0f8f93b1609a327f70b1cd49838e524b
child 313009 6df935dd797009a13ad7e2f3ad1fe9946214c824
push id20479
push userkwierso@gmail.com
push dateThu, 08 Sep 2016 01:08:46 +0000
treeherderfx-team@fb7c6b034329 [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
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
@@ -1234,16 +1236,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;
@@ -1263,32 +1273,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>();
@@ -1885,24 +1913,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.
 
@@ -2592,21 +2612,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) {