Bug 839117 - 'Don't allow a leaking memory reporter to hang workers'. r=jlebar, a=akeybl
authorBen Turner <bent.mozilla@gmail.com>
Fri, 08 Feb 2013 03:50:00 -0800
changeset 127482 c4aa14e5e8ebf9ed79cd13445e50292d52e868b0
parent 127481 b377d89834cb8e3aeb792714293b55bd645ca8ff
child 127483 8d1c80a56d0078d706dc09660e159df4acf9aa8e
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlebar, akeybl
bugs839117
milestone20.0a2
Bug 839117 - 'Don't allow a leaking memory reporter to hang workers'. r=jlebar, a=akeybl
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1435,94 +1435,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,
@@ -1773,16 +1697,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,
@@ -2361,19 +2382,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);
 }
@@ -2820,161 +2840,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;
@@ -334,19 +387,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;
@@ -541,40 +595,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
 
@@ -709,19 +765,16 @@ public:
   UpdateJSRuntimeHeapSizeInternal(JSContext* aCx, uint32_t aJSRuntimeHeapSize);
 
   void
   ScheduleDeletion(bool aWasPending);
 
   bool
   BlockAndCollectRuntimeStats(bool aIsQuick, void* aData);
 
-  void
-  NoteDeadMemoryReporter();
-
   bool
   XHRParamsAllowed() const
   {
     return mXHRParamsAllowed;
   }
 
   void
   SetXHRParamsAllowed(bool aAllowed)