Bug 839117 - 'Don't allow a leaking memory reporter to hang workers'. r=jlebar.
authorBen Turner <bent.mozilla@gmail.com>
Fri, 08 Feb 2013 03:50:00 -0800
changeset 131159 845e50f1ca7618975160db36b6130582b4576d88
parent 131158 3f192cae69e2c7d3087470c6191e2128e00ebb08
child 131160 d32064860f36bbc48e34ceb712f08860d2ad7ed9
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlebar
bugs839117
milestone21.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 839117 - 'Don't allow a leaking memory reporter to hang workers'. r=jlebar.
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1437,94 +1437,18 @@ public:
     aCompartmentStats->extra1 = strdup(cJSPathPrefix.get());
 
     // This should never be used when reporting with workers (hence the "?!").
     static const char bogusMemoryReporterPath[] = "explicit/workers/?!/";
     aCompartmentStats->extra2 = const_cast<char*>(bogusMemoryReporterPath);
   }
 };
 
-class MemoryReporter MOZ_FINAL : public nsIMemoryMultiReporter
-{
-  friend class WorkerPrivate;
-
-  WorkerPrivate* mWorkerPrivate;
-  nsCString mRtPath;
-
-public:
-  NS_DECL_ISUPPORTS
-
-  MemoryReporter(WorkerPrivate* aWorkerPrivate)
-  : mWorkerPrivate(aWorkerPrivate)
-  {
-    aWorkerPrivate->AssertIsOnWorkerThread();
-
-    nsCString escapedDomain(aWorkerPrivate->Domain());
-    escapedDomain.ReplaceChar('/', '\\');
-
-    NS_ConvertUTF16toUTF8 escapedURL(aWorkerPrivate->ScriptURL());
-    escapedURL.ReplaceChar('/', '\\');
-
-    nsAutoCString addressString;
-    addressString.AppendPrintf("0x%p", static_cast<void*>(aWorkerPrivate));
-
-    mRtPath = NS_LITERAL_CSTRING("explicit/workers/workers(") +
-              escapedDomain + NS_LITERAL_CSTRING(")/worker(") +
-              escapedURL + NS_LITERAL_CSTRING(", ") + addressString +
-              NS_LITERAL_CSTRING(")/");
-  }
-
-  ~MemoryReporter()
-  {
-    mWorkerPrivate->NoteDeadMemoryReporter();
-  }
-
-  NS_IMETHOD
-  GetName(nsACString& aName)
-  {
-    aName.AssignLiteral("workers");
-    return NS_OK;
-  }
-
-  NS_IMETHOD
-  CollectReports(nsIMemoryMultiReporterCallback* aCallback,
-                 nsISupports* aClosure)
-  {
-    AssertIsOnMainThread();
-
-    WorkerJSRuntimeStats rtStats(mRtPath);
-    if (!mWorkerPrivate->BlockAndCollectRuntimeStats(/* aIsQuick = */ false,
-                                                     &rtStats)) {
-      return NS_ERROR_FAILURE;
-    }
-
-    // Always report, even if we're disabled, so that we at least get an entry
-    // in about::memory.
-    return xpc::ReportJSRuntimeExplicitTreeStats(rtStats, mRtPath,
-                                                 aCallback, aClosure);
-  }
-
-  NS_IMETHOD
-  GetExplicitNonHeap(int64_t* aAmount)
-  {
-    AssertIsOnMainThread();
-
-    if (!mWorkerPrivate->BlockAndCollectRuntimeStats(/* aIsQuick = */ true,
-                                                     aAmount)) {
-      return NS_ERROR_FAILURE;
-    }
-
-    return NS_OK;
-  }
-};
-
 } /* anonymous namespace */
 
-NS_IMPL_THREADSAFE_ISUPPORTS1(MemoryReporter, nsIMemoryMultiReporter)
-
 #ifdef DEBUG
 void
 mozilla::dom::workers::AssertIsOnMainThread()
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 }
 
 WorkerRunnable::WorkerRunnable(WorkerPrivate* aWorkerPrivate, Target aTarget,
@@ -1775,16 +1699,113 @@ struct WorkerPrivate::TimeoutInfo
   mozilla::TimeDuration mInterval;
   nsCString mFilename;
   uint32_t mLineNumber;
   uint32_t mId;
   bool mIsInterval;
   bool mCanceled;
 };
 
+class WorkerPrivate::MemoryReporter MOZ_FINAL : public nsIMemoryMultiReporter
+{
+  friend class WorkerPrivate;
+
+  SharedMutex mMutex;
+  WorkerPrivate* mWorkerPrivate;
+  nsCString mRtPath;
+
+public:
+  NS_DECL_ISUPPORTS
+
+  MemoryReporter(WorkerPrivate* aWorkerPrivate)
+  : mMutex(aWorkerPrivate->mMutex), mWorkerPrivate(aWorkerPrivate)
+  {
+    aWorkerPrivate->AssertIsOnWorkerThread();
+
+    nsCString escapedDomain(aWorkerPrivate->Domain());
+    escapedDomain.ReplaceChar('/', '\\');
+
+    NS_ConvertUTF16toUTF8 escapedURL(aWorkerPrivate->ScriptURL());
+    escapedURL.ReplaceChar('/', '\\');
+
+    nsAutoCString addressString;
+    addressString.AppendPrintf("0x%p", static_cast<void*>(aWorkerPrivate));
+
+    mRtPath = NS_LITERAL_CSTRING("explicit/workers/workers(") +
+              escapedDomain + NS_LITERAL_CSTRING(")/worker(") +
+              escapedURL + NS_LITERAL_CSTRING(", ") + addressString +
+              NS_LITERAL_CSTRING(")/");
+  }
+
+  NS_IMETHOD
+  GetName(nsACString& aName)
+  {
+    aName.AssignLiteral("workers");
+    return NS_OK;
+  }
+
+  NS_IMETHOD
+  CollectReports(nsIMemoryMultiReporterCallback* aCallback,
+                 nsISupports* aClosure)
+  {
+    AssertIsOnMainThread();
+
+    WorkerJSRuntimeStats rtStats(mRtPath);
+
+    {
+      MutexAutoLock lock(mMutex);
+
+      if (!mWorkerPrivate ||
+          !mWorkerPrivate->BlockAndCollectRuntimeStats(/* aIsQuick = */ false,
+                                                       &rtStats)) {
+        // Returning NS_OK here will effectively report 0 memory.
+        return NS_OK;
+      }
+    }
+
+    return xpc::ReportJSRuntimeExplicitTreeStats(rtStats, mRtPath,
+                                                 aCallback, aClosure);
+  }
+
+  NS_IMETHOD
+  GetExplicitNonHeap(int64_t* aAmount)
+  {
+    AssertIsOnMainThread();
+
+    {
+      MutexAutoLock lock(mMutex);
+
+      if (!mWorkerPrivate ||
+          !mWorkerPrivate->BlockAndCollectRuntimeStats(/* aIsQuick = */ true,
+                                                       aAmount)) {
+        *aAmount = 0;
+      }
+    }
+
+    return NS_OK;
+  }
+
+private:
+  ~MemoryReporter()
+  { }
+
+  void
+  Disable()
+  {
+    // Called from WorkerPrivate::DisableMemoryReporter.
+    mMutex.AssertCurrentThreadOwns();
+
+    NS_ASSERTION(mWorkerPrivate, "Disabled more than once!");
+    mWorkerPrivate = nullptr;
+  }
+};
+
+NS_IMPL_THREADSAFE_ISUPPORTS1(WorkerPrivate::MemoryReporter,
+                              nsIMemoryMultiReporter)
+
 template <class Derived>
 WorkerPrivateParent<Derived>::WorkerPrivateParent(
                                      JSContext* aCx, JSObject* aObject,
                                      WorkerPrivate* aParent,
                                      JSContext* aParentJSContext,
                                      const nsAString& aScriptURL,
                                      bool aIsChromeWorker,
                                      const nsACString& aDomain,
@@ -2380,19 +2401,18 @@ WorkerPrivate::WorkerPrivate(JSContext* 
                              bool aXHRParamsAllowed)
 : WorkerPrivateParent<WorkerPrivate>(aCx, aObject, aParent, aParentJSContext,
                                      aScriptURL, aIsChromeWorker, aDomain,
                                      aWindow, aParentScriptContext, aBaseURI,
                                      aPrincipal, aCSP, aEvalAllowed),
   mJSContext(nullptr), mErrorHandlerRecursionCount(0), mNextTimeoutId(1),
   mStatus(Pending), mSuspended(false), mTimerRunning(false),
   mRunningExpiredTimeouts(false), mCloseHandlerStarted(false),
-  mCloseHandlerFinished(false), mMemoryReporterAlive(false),
-  mMemoryReporterRunning(false), mBlockedForMemoryReporter(false),
-  mXHRParamsAllowed(aXHRParamsAllowed)
+  mCloseHandlerFinished(false), mMemoryReporterRunning(false),
+  mBlockedForMemoryReporter(false), mXHRParamsAllowed(aXHRParamsAllowed)
 {
   MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivate);
 }
 
 WorkerPrivate::~WorkerPrivate()
 {
   MOZ_COUNT_DTOR(mozilla::dom::workers::WorkerPrivate);
 }
@@ -2834,161 +2854,136 @@ WorkerPrivate::ScheduleDeletion(bool aWa
     }
   }
 }
 
 bool
 WorkerPrivate::BlockAndCollectRuntimeStats(bool aIsQuick, void* aData)
 {
   AssertIsOnMainThread();
+  mMutex.AssertCurrentThreadOwns();
   NS_ASSERTION(aData, "Null data!");
 
-  JSRuntime* rt;
-
-  {
-    MutexAutoLock lock(mMutex);
-
-    // If the worker is being torn down then we just bail out now.
-    if (!mMemoryReporter) {
-      if (aIsQuick) {
-        *static_cast<int64_t*>(aData) = 0;
-      }
-      return true;
-    }
-
-    // This signals the worker that it should block itself as soon as possible.
-    mMemoryReporterRunning = true;
-
-    NS_ASSERTION(mJSContext, "This must never be null!");
-    rt = JS_GetRuntime(mJSContext);
-
-    // If the worker is not already blocked (e.g. waiting for a worker event or
-    // currently in a ctypes call) then we need to trigger the operation
-    // callback to trap the worker.
-    if (!mBlockedForMemoryReporter) {
-      JS_TriggerOperationCallback(rt);
-
-      // Wait until the worker actually blocks.
-      while (!mBlockedForMemoryReporter) {
-        mMemoryReportCondVar.Wait();
-      }
+  NS_ASSERTION(!mMemoryReporterRunning, "How can we get reentered here?!");
+
+  // This signals the worker that it should block itself as soon as possible.
+  mMemoryReporterRunning = true;
+
+  NS_ASSERTION(mJSContext, "This must never be null!");
+  JSRuntime* rt = JS_GetRuntime(mJSContext);
+
+  // If the worker is not already blocked (e.g. waiting for a worker event or
+  // currently in a ctypes call) then we need to trigger the operation
+  // callback to trap the worker.
+  if (!mBlockedForMemoryReporter) {
+    JS_TriggerOperationCallback(rt);
+
+    // Wait until the worker actually blocks.
+    while (!mBlockedForMemoryReporter) {
+      mMemoryReportCondVar.Wait();
     }
   }
 
-  bool succeeded;
-
-  // Now do the actual report.
-  if (aIsQuick) {
-    *static_cast<int64_t*>(aData) =
-      JS::GetExplicitNonHeapForRuntime(rt, JsWorkerMallocSizeOf);
-    succeeded = true;
-  } else {
-    succeeded =
-      JS::CollectRuntimeStats(rt, static_cast<JS::RuntimeStats*>(aData),
-                              nullptr);
+  bool succeeded = false;
+
+  // If mMemoryReporter is still set then we can do the actual report. Otherwise
+  // we're trying to shut down and we don't want to do anything but clean up.
+  if (mMemoryReporter) {
+    // Don't hold the lock while doing the actual report.
+    MutexAutoUnlock unlock(mMutex);
+
+    if (aIsQuick) {
+      *static_cast<int64_t*>(aData) =
+        JS::GetExplicitNonHeapForRuntime(rt, JsWorkerMallocSizeOf);
+      succeeded = true;
+    } else {
+      succeeded =
+        JS::CollectRuntimeStats(rt, static_cast<JS::RuntimeStats*>(aData),
+                                nullptr);
+    }
   }
 
-  MutexAutoLock lock(mMutex);
-
   NS_ASSERTION(mMemoryReporterRunning, "This isn't possible!");
   NS_ASSERTION(mBlockedForMemoryReporter, "Somehow we got unblocked!");
 
   // Tell the worker that it can now continue its execution.
   mMemoryReporterRunning = false;
 
   // The worker may be waiting so we must notify.
   mMemoryReportCondVar.Notify();
 
   return succeeded;
 }
 
 void
-WorkerPrivate::NoteDeadMemoryReporter()
-{
-  // This may happen on the worker thread or the main thread.
-  MutexAutoLock lock(mMutex);
-
-  NS_ASSERTION(mMemoryReporterAlive, "Must be alive!");
-
-  // Tell the worker that the memory reporter has died.
-  mMemoryReporterAlive = false;
-
-  // The worker may be waiting so we must notify.
-  mMemoryReportCondVar.Notify();
-}
-
-void
 WorkerPrivate::EnableMemoryReporter()
 {
   AssertIsOnWorkerThread();
 
-  NS_ASSERTION(!mMemoryReporterAlive, "This isn't possible!");
-
   // No need to lock here since the main thread can't race until we've
   // successfully registered the reporter.
-  mMemoryReporterAlive = true;
   mMemoryReporter = new MemoryReporter(this);
 
   if (NS_FAILED(NS_RegisterMemoryMultiReporter(mMemoryReporter))) {
     NS_WARNING("Failed to register memory reporter!");
     // No need to lock here since a failed registration means our memory
     // reporter can't start running. Just clean up.
     mMemoryReporter = nullptr;
 
-    // That should have killed the memory reporter.
-    NS_ASSERTION(!mMemoryReporterAlive, "Must be dead!");
-
     return;
   }
 }
 
 void
 WorkerPrivate::DisableMemoryReporter()
 {
   AssertIsOnWorkerThread();
 
-  // First swap out our memory reporter so that the main thread stops trying to
-  // signal us when it's reporting memory.
-  nsCOMPtr<nsIMemoryMultiReporter> memoryReporter;
+  nsRefPtr<MemoryReporter> memoryReporter;
   {
     MutexAutoLock lock(mMutex);
+
+    // There is nothing to do here if the memory reporter was never successfully
+    // registered.
+    if (!mMemoryReporter) {
+      return;
+    }
+
+    // We don't need this set any longer. Swap it out so that we can unregister
+    // below.
     mMemoryReporter.swap(memoryReporter);
+
+    // Next disable the memory reporter so that the main thread stops trying to
+    // signal us.
+    memoryReporter->Disable();
+
+    // If the memory reporter is waiting to start then we need to wait for it to
+    // finish.
+    if (mMemoryReporterRunning) {
+      NS_ASSERTION(!mBlockedForMemoryReporter,
+                   "Can't be blocked in more than one place at the same time!");
+      mBlockedForMemoryReporter = true;
+
+      // Tell the main thread that we're blocked.
+      mMemoryReportCondVar.Notify();
+
+      // Wait for it the main thread to finish. Since we swapped out
+      // mMemoryReporter above the main thread should respond quickly.
+      while (mMemoryReporterRunning) {
+        mMemoryReportCondVar.Wait();
+      }
+
+      NS_ASSERTION(mBlockedForMemoryReporter, "Somehow we got unblocked!");
+      mBlockedForMemoryReporter = false;
+    }
   }
 
-  // The main thread could immediately try to report memory again here but it
-  // will not actually do anything since we've cleared mMemoryReporter.
-
-  // If we never registered the memory reporter (e.g. some kind of error in
-  // NS_RegisterMemoryMultiReporter) then there's nothing else we need to do
-  // here.
-  if (!memoryReporter) {
-    return;
-  }
-
-  NS_ASSERTION(mMemoryReporterAlive, "Huh?!");
-
-  // Unregister.
+  // Finally unregister the memory reporter.
   if (NS_FAILED(NS_UnregisterMemoryMultiReporter(memoryReporter))) {
-    // If this fails then the memory reporter will probably never die and we'll
-    // hang below waiting for it. If this ever happens we need to fix
-    // NS_UnregisterMemoryMultiReporter.
-    MOZ_NOT_REACHED("Failed to unregister memory reporter! Worker is going to "
-                    "hang!");
-  }
-
-  // Now drop our reference to the memory reporter. The main thread could be
-  // holding another reference so it may not die immediately.
-  memoryReporter = nullptr;
-
-  MutexAutoLock lock(mMutex);
-
-  // Wait once again to make sure that the main thread is done with the memory
-  // reporter.
-  while (mMemoryReporterAlive) {
-    mMemoryReportCondVar.Wait();
+    NS_WARNING("Failed to unregister memory reporter!");
   }
 }
 
 void
 WorkerPrivate::WaitForWorkerEvents(PRIntervalTime aInterval)
 {
   AssertIsOnWorkerThread();
   mMutex.AssertCurrentThreadOwns();
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -10,16 +10,17 @@
 
 #include "nsIContentSecurityPolicy.h"
 #include "nsIRunnable.h"
 #include "nsIThread.h"
 #include "nsIThreadInternal.h"
 #include "nsPIDOMWindow.h"
 
 #include "jsapi.h"
+#include "mozilla/Assertions.h"
 #include "mozilla/CondVar.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/TimeStamp.h"
 #include "nsAutoPtr.h"
 #include "nsCOMPtr.h"
 #include "nsEventQueue.h"
 #include "nsStringGlue.h"
 #include "nsTArray.h"
@@ -143,16 +144,68 @@ protected:
 
   virtual ~WorkerControlRunnable()
   { }
 
   virtual bool
   DispatchInternal();
 };
 
+// SharedMutex is a small wrapper around an (internal) reference-counted Mutex
+// object. It exists to avoid changing a lot of code to use Mutex* instead of
+// Mutex&.
+class SharedMutex
+{
+  typedef mozilla::Mutex Mutex;
+
+  class RefCountedMutex : public Mutex
+  {
+  public:
+    RefCountedMutex(const char* aName)
+    : Mutex(aName)
+    { }
+
+    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(RefCountedMutex)
+
+  private:
+    ~RefCountedMutex()
+    { }
+  };
+
+  nsRefPtr<RefCountedMutex> mMutex;
+
+public:
+  SharedMutex(const char* aName)
+  : mMutex(new RefCountedMutex(aName))
+  { }
+
+  SharedMutex(SharedMutex& aOther)
+  : mMutex(aOther.mMutex)
+  { }
+
+  operator Mutex&()
+  {
+    MOZ_ASSERT(mMutex);
+    return *mMutex;
+  }
+
+  operator const Mutex&() const
+  {
+    MOZ_ASSERT(mMutex);
+    return *mMutex;
+  }
+
+  void
+  AssertCurrentThreadOwns() const
+  {
+    MOZ_ASSERT(mMutex);
+    mMutex->AssertCurrentThreadOwns();
+  }
+};
+
 template <class Derived>
 class WorkerPrivateParent : public EventTarget
 {
 public:
   struct LocationInfo
   {
     nsCString mHref;
     nsCString mProtocol;
@@ -160,17 +213,17 @@ public:
     nsCString mHostname;
     nsCString mPort;
     nsCString mPathname;
     nsCString mSearch;
     nsCString mHash;
   };
 
 protected:
-  mozilla::Mutex mMutex;
+  SharedMutex mMutex;
   mozilla::CondVar mCondVar;
   mozilla::CondVar mMemoryReportCondVar;
 
 private:
   JSObject* mJSObject;
   WorkerPrivate* mParent;
   JSContext* mParentJSContext;
   nsString mScriptURL;
@@ -335,19 +388,20 @@ public:
     return mParentSuspended;
   }
 
   bool
   IsAcceptingEvents()
   {
     AssertIsOnParentThread();
     bool acceptingEvents;
-    mMutex.Lock();
-    acceptingEvents = mParentStatus < Terminating;
-    mMutex.Unlock();
+    {
+      mozilla::MutexAutoLock lock(mMutex);
+      acceptingEvents = mParentStatus < Terminating;
+    }
     return acceptingEvents;
   }
 
   Status
   ParentStatus() const
   {
     mMutex.AssertCurrentThreadOwns();
     return mParentStatus;
@@ -548,40 +602,42 @@ class WorkerPrivate : public WorkerPriva
     {
       WorkerRunnable* event;
       while (mQueue.Pop(event)) {
         event->Release();
       }
     }
   };
 
+  class MemoryReporter;
+  friend class MemoryReporter;
+
   nsTArray<nsAutoPtr<SyncQueue> > mSyncQueues;
 
   // Touched on multiple threads, protected with mMutex.
   JSContext* mJSContext;
   nsRefPtr<WorkerCrossThreadDispatcher> mCrossThreadDispatcher;
 
   // Things touched on worker thread only.
   nsTArray<ParentType*> mChildWorkers;
   nsTArray<WorkerFeature*> mFeatures;
   nsTArray<nsAutoPtr<TimeoutInfo> > mTimeouts;
 
   nsCOMPtr<nsITimer> mTimer;
-  nsCOMPtr<nsIMemoryMultiReporter> mMemoryReporter;
+  nsRefPtr<MemoryReporter> mMemoryReporter;
 
   mozilla::TimeStamp mKillTime;
   uint32_t mErrorHandlerRecursionCount;
   uint32_t mNextTimeoutId;
   Status mStatus;
   bool mSuspended;
   bool mTimerRunning;
   bool mRunningExpiredTimeouts;
   bool mCloseHandlerStarted;
   bool mCloseHandlerFinished;
-  bool mMemoryReporterAlive;
   bool mMemoryReporterRunning;
   bool mBlockedForMemoryReporter;
   bool mXHRParamsAllowed;
 
 #ifdef DEBUG
   nsCOMPtr<nsIThread> mThread;
 #endif
 
@@ -716,19 +772,16 @@ public:
   UpdateJSWorkerMemoryParameterInternal(JSContext* aCx, JSGCParamKey key, uint32_t aValue);
 
   void
   ScheduleDeletion(bool aWasPending);
 
   bool
   BlockAndCollectRuntimeStats(bool aIsQuick, void* aData);
 
-  void
-  NoteDeadMemoryReporter();
-
   bool
   XHRParamsAllowed() const
   {
     return mXHRParamsAllowed;
   }
 
   void
   SetXHRParamsAllowed(bool aAllowed)