Bug 679551 - 'Workers: Deadlock in WorkerPrivate::BlockAndCollectRuntimeStats if worker is blocked (LastPass extension)'. r=mrbkap.

--HG--
extra : transplant_source : 8%93A%E7_%AA2%EC%D3%D2%862%FD%27I%E8%A6%12%8CM

dom/workers/ChromeWorkerScope.cpp

@@ -45,6 +45,10 @@
 #include "nsNativeCharsetUtils.h"
 #include "nsStringGlue.h"
 
+#include "WorkerPrivate.h"
+
+#define CTYPES_STR "ctypes"
+
 USING_WORKERS_NAMESPACE
 
 namespace {
@@ -74,8 +78,57 @@ UnicodeToNative(JSContext* aCx, const jschar* aSource, size_t aSourceLen)
 JSCTypesCallbacks gCTypesCallbacks = {
   UnicodeToNative
 };
+
+JSBool
+CTypesLazyGetter(JSContext* aCx, JSObject* aObj, jsid aId, jsval* aVp)
+{
+  NS_ASSERTION(JS_GetGlobalObject(aCx) == aObj, "Not a global object!");
+  NS_ASSERTION(JSID_IS_STRING(aId), "Bad id!");
+  NS_ASSERTION(JS_FlatStringEqualsAscii(JSID_TO_FLAT_STRING(aId), CTYPES_STR),
+               "Bad id!");
+
+  WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx);
+  NS_ASSERTION(worker->IsChromeWorker(), "This should always be true!");
+
+  if (!worker->DisableMemoryReporter()) {
+    return false;
+  }
+
+  jsval ctypes;
+  return JS_DeletePropertyById(aCx, aObj, aId) &&
+         JS_InitCTypesClass(aCx, aObj) &&
+         JS_GetPropertyById(aCx, aObj, aId, &ctypes) &&
+         JS_SetCTypesCallbacks(aCx, JSVAL_TO_OBJECT(ctypes),
+                               &gCTypesCallbacks) &&
+         JS_GetPropertyById(aCx, aObj, aId, aVp);
+}
 #endif
 
+inline bool
+DefineCTypesLazyGetter(JSContext* aCx, JSObject* aGlobal)
+{
+#ifdef BUILD_CTYPES
+  {
+    JSString* ctypesStr = JS_InternString(aCx, CTYPES_STR);
+    if (!ctypesStr) {
+      return false;
+    }
+
+    jsid ctypesId = INTERNED_STRING_TO_JSID(aCx, ctypesStr);
+
+    // We use a lazy getter here to let us unregister the blocking memory
+    // reporter since ctypes can easily block the worker thread and we can
+    // deadlock. Remove once bug 673323 is fixed.
+    if (!JS_DefinePropertyById(aCx, aGlobal, ctypesId, JSVAL_VOID,
+                               CTypesLazyGetter, NULL, 0)) {
+      return false;
+    }
+  }
+#endif
+
+  return true;
+}
+
 } // anonymous namespace
 
 BEGIN_WORKERS_NAMESPACE
@@ -85,16 +138,8 @@ namespace chromeworker {
 bool
 DefineChromeWorkerFunctions(JSContext* aCx, JSObject* aGlobal)
 {
-#ifdef BUILD_CTYPES
+  // Currently ctypes is the only special property given to ChromeWorkers.
-  jsval ctypes;
+  return DefineCTypesLazyGetter(aCx, aGlobal);
-  if (!JS_InitCTypesClass(aCx, aGlobal) ||
-      !JS_GetProperty(aCx, aGlobal, "ctypes", &ctypes) ||
-      !JS_SetCTypesCallbacks(aCx, JSVAL_TO_OBJECT(ctypes), &gCTypesCallbacks)) {
-    return false;
-  }
-#endif
-
-  return true;
 }
 
 } // namespace chromeworker

dom/workers/RuntimeService.cpp

@@ -46,13 +46,11 @@
 #include "nsIPlatformCharset.h"
 #include "nsIPrincipal.h"
 #include "nsIJSContextStack.h"
-#include "nsIMemoryReporter.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsISupportsPriority.h"
 #include "nsITimer.h"
 #include "nsPIDOMWindow.h"
 
-#include "jsprf.h"
 #include "mozilla/Preferences.h"
 #include "nsContentUtils.h"
 #include "nsDOMJSUtils.h"
@@ -287,61 +285,6 @@ CreateJSContextForWorker(WorkerPrivate* aWorkerPrivate)
   return workerCx;
 }
 
-class WorkerMemoryReporter : public nsIMemoryMultiReporter
-{
-  WorkerPrivate* mWorkerPrivate;
-  nsCString mPathPrefix;
-
-public:
-  NS_DECL_ISUPPORTS
-
-  WorkerMemoryReporter(WorkerPrivate* aWorkerPrivate)
-  : mWorkerPrivate(aWorkerPrivate)
-  {
-    aWorkerPrivate->AssertIsOnWorkerThread();
-
-    nsCString escapedDomain(aWorkerPrivate->Domain());
-    escapedDomain.ReplaceChar('/', '\\');
-
-    NS_ConvertUTF16toUTF8 escapedURL(aWorkerPrivate->ScriptURL());
-    escapedURL.ReplaceChar('/', '\\');
-
-    // 64bit address plus '0x' plus null terminator.
-    char address[21];
-    JSUint32 addressSize =
-      JS_snprintf(address, sizeof(address), "0x%llx", aWorkerPrivate);
-    if (addressSize == JSUint32(-1)) {
-      NS_WARNING("JS_snprintf failed!");
-      address[0] = '\0';
-      addressSize = 0;
-    }
-
-    mPathPrefix = NS_LITERAL_CSTRING("explicit/dom/workers(") +
-                  escapedDomain + NS_LITERAL_CSTRING(")/worker(") +
-                  escapedURL + NS_LITERAL_CSTRING(", ") +
-                  nsDependentCString(address, addressSize) +
-                  NS_LITERAL_CSTRING(")/");
-  }
-
-  NS_IMETHOD
-  CollectReports(nsIMemoryMultiReporterCallback* aCallback,
-                 nsISupports* aClosure)
-  {
-    AssertIsOnMainThread();
-
-    IterateData data;
-    if (!mWorkerPrivate->BlockAndCollectRuntimeStats(&data)) {
-      return NS_ERROR_FAILURE;
-    }
-
-    ReportJSRuntimeStats(data, mPathPrefix, aCallback, aClosure);
-
-    return NS_OK;
-  }
-};
-
-NS_IMPL_THREADSAFE_ISUPPORTS1(WorkerMemoryReporter, nsIMemoryMultiReporter)
-
 class WorkerThreadRunnable : public nsRunnable
 {
   WorkerPrivate* mWorkerPrivate;
@@ -368,25 +311,11 @@ public:
       return NS_ERROR_FAILURE;
     }
 
-    nsRefPtr<WorkerMemoryReporter> reporter =
-      new WorkerMemoryReporter(workerPrivate);
-    if (NS_FAILED(NS_RegisterMemoryMultiReporter(reporter))) {
-      NS_WARNING("Failed to register memory reporter!");
-      reporter = nsnull;
-    }
-
     {
       JSAutoRequest ar(cx);
       workerPrivate->DoRunLoop(cx);
     }
 
-    if (reporter) {
-      if (NS_FAILED(NS_UnregisterMemoryMultiReporter(reporter))) {
-        NS_WARNING("Failed to unregister memory reporter!");
-      }
-      reporter = nsnull;
-    }
-
     JSRuntime* rt = JS_GetRuntime(cx);
 
     // XXX Bug 666963 - CTypes can create another JSContext for use with

dom/workers/WorkerPrivate.cpp

@@ -45,6 +45,7 @@
 #include "nsIDocument.h"
 #include "nsIEffectiveTLDService.h"
 #include "nsIJSContextStack.h"
+#include "nsIMemoryReporter.h"
 #include "nsIScriptError.h"
 #include "nsIScriptGlobalObject.h"
 #include "nsIScriptSecurityManager.h"
@@ -57,6 +58,7 @@
 
 #include "jscntxt.h"
 #include "jsdbgapi.h"
+#include "jsprf.h"
 #include "nsAlgorithm.h"
 #include "nsContentUtils.h"
 #include "nsDOMClassInfo.h"
@@ -140,6 +142,87 @@ SwapToISupportsArray(SmartPtr<T>& aSrc,
   dest->swap(rawSupports);
 }
 
+class WorkerMemoryReporter : public nsIMemoryMultiReporter
+{
+  WorkerPrivate* mWorkerPrivate;
+  nsCString mAddressString;
+  nsCString mPathPrefix;
+
+public:
+  NS_DECL_ISUPPORTS
+
+  WorkerMemoryReporter(WorkerPrivate* aWorkerPrivate)
+  : mWorkerPrivate(aWorkerPrivate)
+  {
+    aWorkerPrivate->AssertIsOnWorkerThread();
+
+    nsCString escapedDomain(aWorkerPrivate->Domain());
+    escapedDomain.ReplaceChar('/', '\\');
+
+    NS_ConvertUTF16toUTF8 escapedURL(aWorkerPrivate->ScriptURL());
+    escapedURL.ReplaceChar('/', '\\');
+
+    {
+      // 64bit address plus '0x' plus null terminator.
+      char address[21];
+      JSUint32 addressSize =
+        JS_snprintf(address, sizeof(address), "0x%llx", aWorkerPrivate);
+      if (addressSize != JSUint32(-1)) {
+        mAddressString.Assign(address, addressSize);
+      }
+      else {
+        NS_WARNING("JS_snprintf failed!");
+        mAddressString.AssignLiteral("<unknown address>");
+      }
+    }
+
+    mPathPrefix = NS_LITERAL_CSTRING("explicit/dom/workers(") +
+                  escapedDomain + NS_LITERAL_CSTRING(")/worker(") +
+                  escapedURL + NS_LITERAL_CSTRING(", ") + mAddressString +
+                  NS_LITERAL_CSTRING(")/");
+  }
+
+  NS_IMETHOD
+  CollectReports(nsIMemoryMultiReporterCallback* aCallback,
+                 nsISupports* aClosure)
+  {
+    AssertIsOnMainThread();
+
+    IterateData data;
+
+    if (mWorkerPrivate) {
+      bool disabled;
+      if (!mWorkerPrivate->BlockAndCollectRuntimeStats(&data, &disabled)) {
+        return NS_ERROR_FAILURE;
+      }
+
+      // Don't ever try to talk to the worker again.
+      if (disabled) {
+#ifdef DEBUG
+        {
+          nsCAutoString message("Unable to report memory for ");
+          if (mWorkerPrivate->IsChromeWorker()) {
+            message.AppendLiteral("Chrome");
+          }
+          message += NS_LITERAL_CSTRING("Worker (") + mAddressString +
+                     NS_LITERAL_CSTRING(")! It is either using ctypes or is in "
+                                        "the process of being destroyed");
+          NS_WARNING(message.get());
+        }
+#endif
+        mWorkerPrivate = nsnull;
+      }
+    }
+
+    // Always report, even if we're disabled, so that we at least get an entry
+    // in about::memory.
+    ReportJSRuntimeStats(data, mPathPrefix, aCallback, aClosure);
+    return NS_OK;
+  }
+};
+
+NS_IMPL_THREADSAFE_ISUPPORTS1(WorkerMemoryReporter, nsIMemoryMultiReporter)
+
 struct WorkerStructuredCloneCallbacks
 {
   static JSObject*
@@ -1282,19 +1365,19 @@ class CollectRuntimeStatsRunnable : public WorkerControlRunnable
   typedef mozilla::Mutex Mutex;
   typedef mozilla::CondVar CondVar;
 
-  Mutex* mMutex;
+  Mutex mMutex;
-  CondVar* mCondVar;
+  CondVar mCondVar;
-  volatile bool* mDoneFlag;
+  volatile bool mDone;
   IterateData* mData;
-  volatile bool* mSucceeded;
+  bool* mSucceeded;
 
 public:
-  CollectRuntimeStatsRunnable(WorkerPrivate* aWorkerPrivate, Mutex* aMutex,
+  CollectRuntimeStatsRunnable(WorkerPrivate* aWorkerPrivate, IterateData* aData,
-                              CondVar* aCondVar, volatile bool* aDoneFlag,
+                              bool* aSucceeded)
-                              IterateData* aData, volatile bool* aSucceeded)
   : WorkerControlRunnable(aWorkerPrivate, WorkerThread, UnchangedBusyCount),
-    mMutex(aMutex), mCondVar(aCondVar), mDoneFlag(aDoneFlag), mData(aData),
+    mMutex("CollectRuntimeStatsRunnable::mMutex"),
-    mSucceeded(aSucceeded)
+    mCondVar(mMutex, "CollectRuntimeStatsRunnable::mCondVar"), mDone(false),
+    mData(aData), mSucceeded(aSucceeded)
   { }
 
   bool
@@ -1311,6 +1394,26 @@ public:
     AssertIsOnMainThread();
   }
 
+  bool
+  DispatchInternal()
+  {
+    AssertIsOnMainThread();
+
+    if (!WorkerControlRunnable::DispatchInternal()) {
+      NS_WARNING("Failed to dispatch runnable!");
+      return false;
+    }
+
+    {
+      MutexAutoLock lock(mMutex);
+      while (!mDone) {
+        mCondVar.Wait();
+      }
+    }
+
+    return true;
+  }
+
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
@@ -1319,12 +1422,9 @@ public:
     *mSucceeded = CollectCompartmentStatsForRuntime(JS_GetRuntime(aCx), mData);
 
     {
-      MutexAutoLock lock(*mMutex);
+      MutexAutoLock lock(mMutex);
-
+      mDone = true;
-      NS_ASSERTION(!*mDoneFlag, "Should be false!");
+      mCondVar.Notify();
-
-      *mDoneFlag = true;
-      mCondVar->Notify();
     }
 
     return true;
@@ -2101,7 +2201,8 @@ WorkerPrivate::WorkerPrivate(JSContext* aCx, JSObject* aObject,
   mJSContext(nsnull), mErrorHandlerRecursionCount(0), mNextTimeoutId(1),
   mStatus(Pending), mSuspended(false), mTimerRunning(false),
   mRunningExpiredTimeouts(false), mCloseHandlerStarted(false),
-  mCloseHandlerFinished(false)
+  mCloseHandlerFinished(false), mMemoryReporterRunning(false),
+  mMemoryReporterDisabled(false)
 {
   MOZ_COUNT_CTOR(mozilla::dom::workers::WorkerPrivate);
 }
@@ -2308,6 +2409,13 @@ WorkerPrivate::DoRunLoop(JSContext* aCx)
     mStatus = Running;
   }
 
+  mMemoryReporter = new WorkerMemoryReporter(this);
+
+  if (NS_FAILED(NS_RegisterMemoryMultiReporter(mMemoryReporter))) {
+    NS_WARNING("Failed to register memory reporter!");
+    mMemoryReporter = nsnull;
+  }
+
   for (;;) {
     Status currentStatus;
     nsIRunnable* event;
@@ -2358,6 +2466,17 @@ WorkerPrivate::DoRunLoop(JSContext* aCx)
 
       // If we're supposed to die then we should exit the loop.
       if (currentStatus == Killing) {
+        // Call this before unregistering the reporter as we may be racing with
+        // the main thread.
+        DisableMemoryReporter();
+
+        if (mMemoryReporter) {
+          if (NS_FAILED(NS_UnregisterMemoryMultiReporter(mMemoryReporter))) {
+            NS_WARNING("Failed to unregister memory reporter!");
+          }
+          mMemoryReporter = nsnull;
+        }
+
         StopAcceptingEvents();
         return;
       }
@@ -2376,21 +2495,7 @@ WorkerPrivate::OperationCallback(JSContext* aCx)
 
   for (;;) {
     // Run all control events now.
-    for (;;) {
+    mayContinue = ProcessAllControlRunnables();
-      nsIRunnable* event;
-      {
-        MutexAutoLock lock(mMutex);
-        if (!mControlQueue.Pop(event)) {
-          break;
-        }
-      }
-
-      if (NS_FAILED(event->Run())) {
-        mayContinue = false;
-      }
-
-      NS_RELEASE(event);
-    }
 
     if (!mayContinue || !mSuspended) {
       break;
@@ -2457,35 +2562,86 @@ WorkerPrivate::ScheduleDeletion(bool aWasPending)
 }
 
 bool
-WorkerPrivate::BlockAndCollectRuntimeStats(IterateData* aData)
+WorkerPrivate::BlockAndCollectRuntimeStats(IterateData* aData, bool* aDisabled)
 {
   AssertIsOnMainThread();
-  mMutex.AssertNotCurrentThreadOwns();
   NS_ASSERTION(aData, "Null data!");
 
-  mozilla::Mutex mutex("BlockAndCollectRuntimeStats mutex");
+  {
-  mozilla::CondVar condvar(mutex, "BlockAndCollectRuntimeStats condvar");
+    MutexAutoLock lock(mMutex);
-  volatile bool doneFlag = false;
+
-  volatile bool succeeded = false;
+    if (mMemoryReporterDisabled) {
+      *aDisabled = true;
+      return true;
+    }
+
+    *aDisabled = false;
+    mMemoryReporterRunning = true;
+  }
+
+  bool succeeded;
 
   nsRefPtr<CollectRuntimeStatsRunnable> runnable =
-    new CollectRuntimeStatsRunnable(this, &mutex, &condvar, &doneFlag, aData,
+    new CollectRuntimeStatsRunnable(this, aData, &succeeded);
-                                    &succeeded);
   if (!runnable->Dispatch(nsnull)) {
     NS_WARNING("Failed to dispatch runnable!");
-    return false;
+    succeeded = false;
   }
 
   {
-    MutexAutoLock lock(mutex);
+    MutexAutoLock lock(mMutex);
-    while (!doneFlag) {
+    mMemoryReporterRunning = false;
-      condvar.Wait();
-    }
   }
 
   return succeeded;
 }
 
+bool
+WorkerPrivate::DisableMemoryReporter()
+{
+  AssertIsOnWorkerThread();
+
+  bool result = true;
+
+  {
+    MutexAutoLock lock(mMutex);
+
+    mMemoryReporterDisabled = true;
+
+    while (mMemoryReporterRunning) {
+      MutexAutoUnlock unlock(mMutex);
+      result = ProcessAllControlRunnables() && result;
+    }
+  }
+
+  return result;
+}
+
+bool
+WorkerPrivate::ProcessAllControlRunnables()
+{
+  AssertIsOnWorkerThread();
+
+  bool result = true;
+
+  for (;;) {
+    nsIRunnable* event;
+    {
+      MutexAutoLock lock(mMutex);
+      if (!mControlQueue.Pop(event)) {
+        break;
+      }
+    }
+
+    if (NS_FAILED(event->Run())) {
+      result = false;
+    }
+
+    NS_RELEASE(event);
+  }
+  return result;
+}
+
 bool
 WorkerPrivate::Dispatch(WorkerRunnable* aEvent, EventQueue* aQueue)
 {

dom/workers/WorkerPrivate.h

@@ -63,6 +63,7 @@
 class JSAutoStructuredCloneBuffer;
 class nsIDocument;
 class nsIPrincipal;
+class nsIMemoryMultiReporter;
 class nsIScriptContext;
 class nsIURI;
 class nsPIDOMWindow;
@@ -512,6 +513,7 @@ class WorkerPrivate : public WorkerPrivateParent<WorkerPrivate>
   nsTArray<nsAutoPtr<TimeoutInfo> > mTimeouts;
 
   nsCOMPtr<nsITimer> mTimer;
+  nsCOMPtr<nsIMemoryMultiReporter> mMemoryReporter;
 
   mozilla::TimeStamp mKillTime;
   PRUint32 mErrorHandlerRecursionCount;
@@ -522,6 +524,8 @@ class WorkerPrivate : public WorkerPrivateParent<WorkerPrivate>
   bool mRunningExpiredTimeouts;
   bool mCloseHandlerStarted;
   bool mCloseHandlerFinished;
+  bool mMemoryReporterRunning;
+  bool mMemoryReporterDisabled;
 
 #ifdef DEBUG
   nsCOMPtr<nsIThread> mThread;
@@ -654,7 +658,11 @@ public:
   ScheduleDeletion(bool aWasPending);
 
   bool
-  BlockAndCollectRuntimeStats(mozilla::xpconnect::memory::IterateData* aData);
+  BlockAndCollectRuntimeStats(mozilla::xpconnect::memory::IterateData* aData,
+                              bool* aDisabled);
+
+  bool
+  DisableMemoryReporter();
 
 #ifdef JS_GC_ZEAL
   void
@@ -743,6 +751,9 @@ private:
     ClearQueue(&mQueue);
     ClearQueue(&mControlQueue);
   }
+
+  bool
+  ProcessAllControlRunnables();
 };
 
 WorkerPrivate*

dom/workers/test/chromeWorker_worker.js

@@ -6,6 +6,11 @@ if (!("ctypes" in self)) {
   throw "No ctypes!";
 }
 
+// Go ahead and verify that the ctypes lazy getter actually works.
+if (ctypes.toString() != "[object ctypes]") {
+  throw "Bad ctypes object: " + ctypes.toString();
+}
+
 onmessage = function(event) {
   let worker = new ChromeWorker("chromeWorker_subworker.js");
   worker.onmessage = function(event) {