From a7a4afbef1621b2debdb485905e568e59a9323e0 Mon Sep 17 00:00:00 2001 From: Ben Turner Date: Thu, 8 Sep 2011 17:03:03 -0700 Subject: [PATCH] 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 | 65 ++++++- dom/workers/RuntimeService.cpp | 71 ------- dom/workers/WorkerPrivate.cpp | 244 +++++++++++++++++++----- dom/workers/WorkerPrivate.h | 13 +- dom/workers/test/chromeWorker_worker.js | 5 + 5 files changed, 272 insertions(+), 126 deletions(-) diff --git a/dom/workers/ChromeWorkerScope.cpp b/dom/workers/ChromeWorkerScope.cpp index 5f253dac2f4..296ac6cadb9 100644 --- a/dom/workers/ChromeWorkerScope.cpp +++ b/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 - jsval ctypes; - if (!JS_InitCTypesClass(aCx, aGlobal) || - !JS_GetProperty(aCx, aGlobal, "ctypes", &ctypes) || - !JS_SetCTypesCallbacks(aCx, JSVAL_TO_OBJECT(ctypes), &gCTypesCallbacks)) { - return false; - } -#endif - - return true; + // Currently ctypes is the only special property given to ChromeWorkers. + return DefineCTypesLazyGetter(aCx, aGlobal); } } // namespace chromeworker diff --git a/dom/workers/RuntimeService.cpp b/dom/workers/RuntimeService.cpp index 4a5ee9dc25b..a6dd441d5bc 100644 --- a/dom/workers/RuntimeService.cpp +++ b/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 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 diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index f3298a33cca..07d0d0afb72 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/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& 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(""); + } + } + + 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; - CondVar* mCondVar; - volatile bool* mDoneFlag; + Mutex mMutex; + CondVar mCondVar; + volatile bool mDone; IterateData* mData; - volatile bool* mSucceeded; + bool* mSucceeded; public: - CollectRuntimeStatsRunnable(WorkerPrivate* aWorkerPrivate, Mutex* aMutex, - CondVar* aCondVar, volatile bool* aDoneFlag, - IterateData* aData, volatile bool* aSucceeded) + CollectRuntimeStatsRunnable(WorkerPrivate* aWorkerPrivate, IterateData* aData, + bool* aSucceeded) : WorkerControlRunnable(aWorkerPrivate, WorkerThread, UnchangedBusyCount), - mMutex(aMutex), mCondVar(aCondVar), mDoneFlag(aDoneFlag), mData(aData), - mSucceeded(aSucceeded) + mMutex("CollectRuntimeStatsRunnable::mMutex"), + 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); - - NS_ASSERTION(!*mDoneFlag, "Should be false!"); - - *mDoneFlag = true; - mCondVar->Notify(); + MutexAutoLock lock(mMutex); + mDone = 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 (;;) { - nsIRunnable* event; - { - MutexAutoLock lock(mMutex); - if (!mControlQueue.Pop(event)) { - break; - } - } - - if (NS_FAILED(event->Run())) { - mayContinue = false; - } - - NS_RELEASE(event); - } + mayContinue = ProcessAllControlRunnables(); 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"); - volatile bool doneFlag = false; - volatile bool succeeded = false; + { + MutexAutoLock lock(mMutex); + + if (mMemoryReporterDisabled) { + *aDisabled = true; + return true; + } + + *aDisabled = false; + mMemoryReporterRunning = true; + } + + bool succeeded; nsRefPtr runnable = - new CollectRuntimeStatsRunnable(this, &mutex, &condvar, &doneFlag, aData, - &succeeded); + new CollectRuntimeStatsRunnable(this, aData, &succeeded); if (!runnable->Dispatch(nsnull)) { NS_WARNING("Failed to dispatch runnable!"); - return false; + succeeded = false; } { - MutexAutoLock lock(mutex); - while (!doneFlag) { - condvar.Wait(); - } + MutexAutoLock lock(mMutex); + mMemoryReporterRunning = false; } 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) { diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h index cc31a26ea54..816d5dab1c2 100644 --- a/dom/workers/WorkerPrivate.h +++ b/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 nsTArray > mTimeouts; nsCOMPtr mTimer; + nsCOMPtr mMemoryReporter; mozilla::TimeStamp mKillTime; PRUint32 mErrorHandlerRecursionCount; @@ -522,6 +524,8 @@ class WorkerPrivate : public WorkerPrivateParent bool mRunningExpiredTimeouts; bool mCloseHandlerStarted; bool mCloseHandlerFinished; + bool mMemoryReporterRunning; + bool mMemoryReporterDisabled; #ifdef DEBUG nsCOMPtr 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* diff --git a/dom/workers/test/chromeWorker_worker.js b/dom/workers/test/chromeWorker_worker.js index 936188c2cd7..5738a9ae5df 100644 --- a/dom/workers/test/chromeWorker_worker.js +++ b/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) {