From c969a1e2c0823a58170c28409c9f432ee844dc84 Mon Sep 17 00:00:00 2001 From: Vicamo Yang Date: Fri, 11 Apr 2014 22:27:55 +0800 Subject: [PATCH] Bug 977995 - remove mNetdWorker from SystemWorkerManager. r=khuey, f=vchang There are multiple defects in NetworkWorker and the related parts since the C++ rewrite. 1) NetworkService holds a reference to NetworkWorker and never releases it. It has to wait until the cycle collector comes up to resolve their ownership loop and free NetworkWorker manually. However 2) nsINetworkWorker::shutdown is never called, and that leaves everything living till the end, inclusive of that gNetdConsumer in Netd.cpp. 3) when GC comes to free NetworkWorker, it calls its parent destructor ~NetConsumer(), which in turn calls ~RefCounted(). Having a valid gNetdConsumer in Netd.cpp follows its refCnt is not zero and this triggers an assertion in ~RefCounted(). So, some obvious treatments here. A) NetworkService should call nsINetworkWorker::shutdown upon receiving a shutdown observer event and release the reference to NetworkWorker. B) NetworkWorker should never be double ref-counted. Move NetdConsumer implementation into a separated class. --- dom/system/gonk/NetworkService.js | 13 ++--- dom/system/gonk/NetworkWorker.cpp | 70 +++++++++++++++++-------- dom/system/gonk/NetworkWorker.h | 5 +- dom/system/gonk/SystemWorkerManager.cpp | 18 ------- dom/system/gonk/SystemWorkerManager.h | 2 - 5 files changed, 53 insertions(+), 55 deletions(-) diff --git a/dom/system/gonk/NetworkService.js b/dom/system/gonk/NetworkService.js index a66dbb4815dc..46bee17bfcc2 100644 --- a/dom/system/gonk/NetworkService.js +++ b/dom/system/gonk/NetworkService.js @@ -64,7 +64,6 @@ function NetworkService() { self.handleWorkerMessage(event); } }; - this.worker = gNetworkWorker; gNetworkWorker.start(networkListener); } // Callbacks to invoke when a reply arrives from the net_worker. @@ -80,13 +79,7 @@ NetworkService.prototype = { contractID: NETWORKSERVICE_CONTRACTID, classDescription: "Network Service", interfaces: [Ci.nsINetworkService]}), - QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkService, - Ci.nsISupportsWeakReference, - Ci.nsIWorkerHolder]), - - // nsIWorkerHolder - - worker: null, + QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkService]), // Helpers @@ -551,6 +544,10 @@ NetworkService.prototype = { debug("NetworkService shutdown"); this.shutdown = true; Services.obs.removeObserver(this, "xpcom-shutdown"); + if (gNetworkWorker) { + gNetworkWorker.shutdown(); + gNetworkWorker = null; + } break; } }, diff --git a/dom/system/gonk/NetworkWorker.cpp b/dom/system/gonk/NetworkWorker.cpp index ade352cc6053..1f35052e3f23 100644 --- a/dom/system/gonk/NetworkWorker.cpp +++ b/dom/system/gonk/NetworkWorker.cpp @@ -19,6 +19,8 @@ using namespace mozilla::ipc; namespace mozilla { +nsCOMPtr gWorkerThread; + // The singleton network worker, to be used on the main thread. StaticRefPtr gNetworkWorker; @@ -56,6 +58,7 @@ public: NS_IMETHOD Run() { MOZ_ASSERT(NS_IsMainThread()); + if (gNetworkWorker) { gNetworkWorker->DispatchNetworkResult(mResult); } @@ -77,6 +80,8 @@ public: NS_IMETHOD Run() { + MOZ_ASSERT(!NS_IsMainThread()); + if (gNetworkUtils) { gNetworkUtils->ExecuteCommand(mParams); } @@ -92,10 +97,14 @@ class NetdEventRunnable : public nsRunnable public: NetdEventRunnable(NetdCommand* aCommand) : mCommand(aCommand) - { } + { + MOZ_ASSERT(!NS_IsMainThread()); + } NS_IMETHOD Run() { + MOZ_ASSERT(!NS_IsMainThread()); + if (gNetworkUtils) { gNetworkUtils->onNetdMessage(mCommand); } @@ -106,6 +115,25 @@ private: nsAutoPtr mCommand; }; +class NetdMessageConsumer : public NetdConsumer +{ +public: + NetdMessageConsumer() + { + MOZ_ASSERT(NS_IsMainThread()); + } + + void MessageReceived(NetdCommand* aCommand) + { + MOZ_ASSERT(!NS_IsMainThread()); + + nsCOMPtr runnable = new NetdEventRunnable(aCommand); + if (gWorkerThread) { + gWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); + } + } +}; + NS_IMPL_ISUPPORTS1(NetworkWorker, nsINetworkWorker) NetworkWorker::NetworkWorker() @@ -117,6 +145,7 @@ NetworkWorker::NetworkWorker() NetworkWorker::~NetworkWorker() { MOZ_ASSERT(!gNetworkWorker); + MOZ_ASSERT(!mListener); } already_AddRefed @@ -146,19 +175,19 @@ NetworkWorker::Start(nsINetworkEventListener* aListener) MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(aListener); - nsresult rv; - - if (gNetworkWorker) { - StartNetd(gNetworkWorker); + if (mListener) { + return NS_OK; } - rv = NS_NewThread(getter_AddRefs(mWorkerThread)); + nsresult rv; + + rv = NS_NewNamedThread("NetworkWorker", getter_AddRefs(gWorkerThread)); if (NS_FAILED(rv)) { NS_WARNING("Can't create network control thread"); - Shutdown(); return NS_ERROR_FAILURE; } + StartNetd(new NetdMessageConsumer()); mListener = aListener; return NS_OK; @@ -169,12 +198,15 @@ NetworkWorker::Shutdown() { MOZ_ASSERT(NS_IsMainThread()); + if (!mListener) { + return NS_OK; + } + StopNetd(); - if (mWorkerThread) { - mWorkerThread->Shutdown(); - mWorkerThread = nullptr; - } + gWorkerThread->Shutdown(); + gWorkerThread = nullptr; + mListener = nullptr; return NS_OK; } @@ -194,8 +226,8 @@ NetworkWorker::PostMessage(JS::Handle aOptions, JSContext* aCx) // Dispatch the command to the control thread. NetworkParams NetworkParams(options); nsCOMPtr runnable = new NetworkCommandDispatcher(NetworkParams); - if (mWorkerThread) { - mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); + if (gWorkerThread) { + gWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); } return NS_OK; } @@ -218,20 +250,12 @@ NetworkWorker::DispatchNetworkResult(const NetworkResultOptions& aOptions) } } -// Callback function from Netd, dispatch result to network worker thread. -void -NetworkWorker::MessageReceived(NetdCommand* aCommand) -{ - nsCOMPtr runnable = new NetdEventRunnable(aCommand); - if (mWorkerThread) { - mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); - } -} - // Callback function from network worker thread to update result on main thread. void NetworkWorker::NotifyResult(NetworkResultOptions& aResult) { + MOZ_ASSERT(!NS_IsMainThread()); + nsCOMPtr runnable = new NetworkResultDispatcher(aResult); NS_DispatchToMainThread(runnable); } diff --git a/dom/system/gonk/NetworkWorker.h b/dom/system/gonk/NetworkWorker.h index a98fd4712e48..32b3ae34b087 100644 --- a/dom/system/gonk/NetworkWorker.h +++ b/dom/system/gonk/NetworkWorker.h @@ -13,8 +13,7 @@ namespace mozilla { -class NetworkWorker MOZ_FINAL : public nsINetworkWorker, - public mozilla::ipc::NetdConsumer +class NetworkWorker MOZ_FINAL : public nsINetworkWorker { public: NS_DECL_ISUPPORTS @@ -23,7 +22,6 @@ public: static already_AddRefed FactoryCreate(); void DispatchNetworkResult(const mozilla::dom::NetworkResultOptions& aOptions); - void MessageReceived(mozilla::ipc::NetdCommand* aMessage); private: NetworkWorker(); @@ -31,7 +29,6 @@ private: static void NotifyResult(mozilla::dom::NetworkResultOptions& aResult); - nsCOMPtr mWorkerThread; nsCOMPtr mListener; }; diff --git a/dom/system/gonk/SystemWorkerManager.cpp b/dom/system/gonk/SystemWorkerManager.cpp index f0deb57be155..6fbaac4eacc4 100644 --- a/dom/system/gonk/SystemWorkerManager.cpp +++ b/dom/system/gonk/SystemWorkerManager.cpp @@ -94,8 +94,6 @@ SystemWorkerManager::Init() InitAutoMounter(); InitializeTimeZoneSettingObserver(); - rv = InitNetd(cx); - NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID); @@ -131,8 +129,6 @@ SystemWorkerManager::Shutdown() NfcConsumer::Shutdown(); #endif - mNetdWorker = nullptr; - nsCOMPtr wifi(do_QueryInterface(mWifiWorker)); if (wifi) { wifi->Shutdown(); @@ -184,11 +180,6 @@ SystemWorkerManager::GetInterface(const nsIID &aIID, void **aResult) reinterpret_cast(aResult)); } - if (aIID.Equals(NS_GET_IID(nsINetworkService))) { - return CallQueryInterface(mNetdWorker, - reinterpret_cast(aResult)); - } - NS_WARNING("Got nothing for the requested IID!"); return NS_ERROR_NO_INTERFACE; } @@ -238,15 +229,6 @@ SystemWorkerManager::RegisterNfcWorker(JS::Handle aWorker, #endif // MOZ_NFC } -nsresult -SystemWorkerManager::InitNetd(JSContext *cx) -{ - nsCOMPtr worker = do_GetService("@mozilla.org/network/service;1"); - NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE); - mNetdWorker = worker; - return NS_OK; -} - nsresult SystemWorkerManager::InitWifi(JSContext *cx) { diff --git a/dom/system/gonk/SystemWorkerManager.h b/dom/system/gonk/SystemWorkerManager.h index 1562a80eb758..8ccecf4129b8 100644 --- a/dom/system/gonk/SystemWorkerManager.h +++ b/dom/system/gonk/SystemWorkerManager.h @@ -59,11 +59,9 @@ private: SystemWorkerManager(); ~SystemWorkerManager(); - nsresult InitNetd(JSContext *cx); nsresult InitWifi(JSContext *cx); nsresult InitKeyStore(JSContext *cx); - nsCOMPtr mNetdWorker; nsCOMPtr mWifiWorker; nsRefPtr mKeyStore;