Bug 977995 - remove mNetdWorker from SystemWorkerManager. r=khuey, f=vchang
authorVicamo Yang <vyang@mozilla.com>
Fri, 11 Apr 2014 22:27:55 +0800
changeset 178044 9df1bf74a0c6a61f24fa4c1748c414cda30a012e
parent 178043 9778da56668bcae44daf89856bffccc21b67d17a
child 178045 8e6b5a013750f289c43fe657c7987c6b52377082
push id26572
push userryanvm@gmail.com
push dateFri, 11 Apr 2014 19:46:36 +0000
treeherdermozilla-central@0c2edd35da4d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs977995
milestone31.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 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<NetdConsumer>(). Having a valid gNetdConsumer in Netd.cpp follows its refCnt is not zero and this triggers an assertion in ~RefCounted<NetdConsumer>(). 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
dom/system/gonk/NetworkWorker.cpp
dom/system/gonk/NetworkWorker.h
dom/system/gonk/SystemWorkerManager.cpp
dom/system/gonk/SystemWorkerManager.h
--- a/dom/system/gonk/NetworkService.js
+++ b/dom/system/gonk/NetworkService.js
@@ -59,39 +59,32 @@ function NetworkService() {
   let self = this;
 
   if (gNetworkWorker) {
     let networkListener = {
       onEvent: function(event) {
         self.handleWorkerMessage(event);
       }
     };
-    this.worker = gNetworkWorker;
     gNetworkWorker.start(networkListener);
   }
   // Callbacks to invoke when a reply arrives from the net_worker.
   this.controlCallbacks = Object.create(null);
 
   this.shutdown = false;
   Services.obs.addObserver(this, "xpcom-shutdown", false);
 }
 
 NetworkService.prototype = {
   classID:   NETWORKSERVICE_CID,
   classInfo: XPCOMUtils.generateCI({classID: NETWORKSERVICE_CID,
                                     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
 
   idgen: 0,
   controlMessage: function(params, callback) {
     if (this.shutdown) {
       return;
     }
@@ -546,14 +539,18 @@ NetworkService.prototype = {
   shutdown: false,
 
   observe: function observe(aSubject, aTopic, aData) {
     switch (aTopic) {
       case "xpcom-shutdown":
         debug("NetworkService shutdown");
         this.shutdown = true;
         Services.obs.removeObserver(this, "xpcom-shutdown");
+        if (gNetworkWorker) {
+          gNetworkWorker.shutdown();
+          gNetworkWorker = null;
+        }
         break;
     }
   },
 };
 
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([NetworkService]);
--- a/dom/system/gonk/NetworkWorker.cpp
+++ b/dom/system/gonk/NetworkWorker.cpp
@@ -14,16 +14,18 @@
   { 0x6df093e1, 0x8127, 0x4fa7, {0x90, 0x13, 0xa3, 0xaa, 0xa7, 0x79, 0xbb, 0xdd} }
 
 using namespace mozilla;
 using namespace mozilla::dom;
 using namespace mozilla::ipc;
 
 namespace mozilla {
 
+nsCOMPtr<nsIThread> gWorkerThread;
+
 // The singleton network worker, to be used on the main thread.
 StaticRefPtr<NetworkWorker> gNetworkWorker;
 
 // The singleton networkutils class, that can be used on any thread.
 static nsAutoPtr<NetworkUtils> gNetworkUtils;
 
 // Runnable used dispatch command result on the main thread.
 class NetworkResultDispatcher : public nsRunnable
@@ -51,16 +53,17 @@ public:
     COPY_FIELD(mCurExternalIfname)
     COPY_FIELD(mCurInternalIfname)
 #undef COPY_FIELD
   }
 
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread());
+
     if (gNetworkWorker) {
       gNetworkWorker->DispatchNetworkResult(mResult);
     }
     return NS_OK;
   }
 private:
   NetworkResultOptions mResult;
 };
@@ -72,56 +75,82 @@ public:
   NetworkCommandDispatcher(const NetworkParams& aParams)
     : mParams(aParams)
   {
     MOZ_ASSERT(NS_IsMainThread());
   }
 
   NS_IMETHOD Run()
   {
+    MOZ_ASSERT(!NS_IsMainThread());
+
     if (gNetworkUtils) {
       gNetworkUtils->ExecuteCommand(mParams);
     }
     return NS_OK;
   }
 private:
   NetworkParams mParams;
 };
 
 // Runnable used dispatch netd result on the worker thread.
 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);
     }
     return NS_OK;
   }
 
 private:
   nsAutoPtr<NetdCommand> mCommand;
 };
 
+class NetdMessageConsumer : public NetdConsumer
+{
+public:
+  NetdMessageConsumer()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+  }
+
+  void MessageReceived(NetdCommand* aCommand)
+  {
+    MOZ_ASSERT(!NS_IsMainThread());
+
+    nsCOMPtr<nsIRunnable> runnable = new NetdEventRunnable(aCommand);
+    if (gWorkerThread) {
+      gWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
+    }
+  }
+};
+
 NS_IMPL_ISUPPORTS1(NetworkWorker, nsINetworkWorker)
 
 NetworkWorker::NetworkWorker()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!gNetworkWorker);
 }
 
 NetworkWorker::~NetworkWorker()
 {
   MOZ_ASSERT(!gNetworkWorker);
+  MOZ_ASSERT(!mListener);
 }
 
 already_AddRefed<NetworkWorker>
 NetworkWorker::FactoryCreate()
 {
   if (XRE_GetProcessType() != GeckoProcessType_Default) {
     return nullptr;
   }
@@ -141,45 +170,48 @@ NetworkWorker::FactoryCreate()
 }
 
 NS_IMETHODIMP
 NetworkWorker::Start(nsINetworkEventListener* aListener)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aListener);
 
+  if (mListener) {
+    return NS_OK;
+  }
+
   nsresult rv;
 
-  if (gNetworkWorker) {
-    StartNetd(gNetworkWorker);
-  }
-
-  rv = NS_NewThread(getter_AddRefs(mWorkerThread));
+  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;
 }
 
 NS_IMETHODIMP
 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;
 }
 
 // Receive command from main thread (NetworkService.js).
 NS_IMETHODIMP
 NetworkWorker::PostMessage(JS::Handle<JS::Value> aOptions, JSContext* aCx)
 {
@@ -189,18 +221,18 @@ NetworkWorker::PostMessage(JS::Handle<JS
   if (!options.Init(aCx, aOptions)) {
     NS_WARNING("Bad dictionary passed to NetworkWorker::SendCommand");
     return NS_ERROR_FAILURE;
   }
 
   // Dispatch the command to the control thread.
   NetworkParams NetworkParams(options);
   nsCOMPtr<nsIRunnable> runnable = new NetworkCommandDispatcher(NetworkParams);
-  if (mWorkerThread) {
-    mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
+  if (gWorkerThread) {
+    gWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
   }
   return NS_OK;
 }
 
 void
 NetworkWorker::DispatchNetworkResult(const NetworkResultOptions& aOptions)
 {
   MOZ_ASSERT(NS_IsMainThread());
@@ -213,30 +245,22 @@ NetworkWorker::DispatchNetworkResult(con
   }
 
   // Call the listener with a JS value.
   if (mListener) {
     mListener->OnEvent(val);
   }
 }
 
-// Callback function from Netd, dispatch result to network worker thread.
-void
-NetworkWorker::MessageReceived(NetdCommand* aCommand)
-{
-  nsCOMPtr<nsIRunnable> 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<nsIRunnable> runnable = new NetworkResultDispatcher(aResult);
   NS_DispatchToMainThread(runnable);
 }
 
 NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(NetworkWorker,
                                          NetworkWorker::FactoryCreate)
 
 NS_DEFINE_NAMED_CID(NS_NETWORKWORKER_CID);
--- a/dom/system/gonk/NetworkWorker.h
+++ b/dom/system/gonk/NetworkWorker.h
@@ -8,33 +8,30 @@
 #include "mozilla/dom/NetworkOptionsBinding.h"
 #include "mozilla/ipc/Netd.h"
 #include "nsINetworkWorker.h"
 #include "nsCOMPtr.h"
 #include "nsThread.h"
 
 namespace mozilla {
 
-class NetworkWorker MOZ_FINAL : public nsINetworkWorker,
-                                public mozilla::ipc::NetdConsumer
+class NetworkWorker MOZ_FINAL : public nsINetworkWorker
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSINETWORKWORKER
 
   static already_AddRefed<NetworkWorker> FactoryCreate();
 
   void DispatchNetworkResult(const mozilla::dom::NetworkResultOptions& aOptions);
-  void MessageReceived(mozilla::ipc::NetdCommand* aMessage);
 
 private:
   NetworkWorker();
   ~NetworkWorker();
 
   static void NotifyResult(mozilla::dom::NetworkResultOptions& aResult);
 
-  nsCOMPtr<nsIThread> mWorkerThread;
   nsCOMPtr<nsINetworkEventListener> mListener;
 };
 
 } // namespace mozilla
 
 #endif  // NetworkWorker_h
--- a/dom/system/gonk/SystemWorkerManager.cpp
+++ b/dom/system/gonk/SystemWorkerManager.cpp
@@ -89,18 +89,16 @@ SystemWorkerManager::Init()
     NS_WARNING("Failed to initialize WiFi Networking!");
     return rv;
   }
 
   InitKeyStore(cx);
 
   InitAutoMounter();
   InitializeTimeZoneSettingObserver();
-  rv = InitNetd(cx);
-  NS_ENSURE_SUCCESS(rv, rv);
   nsCOMPtr<nsIAudioManager> audioManager =
     do_GetService(NS_AUDIOMANAGER_CONTRACTID);
 
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
   if (!obs) {
     NS_WARNING("Failed to get observer service!");
     return NS_ERROR_FAILURE;
   }
@@ -126,18 +124,16 @@ SystemWorkerManager::Shutdown()
 #ifdef MOZ_B2G_RIL
   RilConsumer::Shutdown();
 #endif
 
 #ifdef MOZ_NFC
   NfcConsumer::Shutdown();
 #endif
 
-  mNetdWorker = nullptr;
-
   nsCOMPtr<nsIWifi> wifi(do_QueryInterface(mWifiWorker));
   if (wifi) {
     wifi->Shutdown();
     wifi = nullptr;
   }
   mWifiWorker = nullptr;
 
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
@@ -179,21 +175,16 @@ SystemWorkerManager::GetInterface(const 
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
   if (aIID.Equals(NS_GET_IID(nsIWifi))) {
     return CallQueryInterface(mWifiWorker,
                               reinterpret_cast<nsIWifi**>(aResult));
   }
 
-  if (aIID.Equals(NS_GET_IID(nsINetworkService))) {
-    return CallQueryInterface(mNetdWorker,
-                              reinterpret_cast<nsINetworkService**>(aResult));
-  }
-
   NS_WARNING("Got nothing for the requested IID!");
   return NS_ERROR_NO_INTERFACE;
 }
 
 nsresult
 SystemWorkerManager::RegisterRilWorker(unsigned int aClientId,
                                        JS::Handle<JS::Value> aWorker,
                                        JSContext *aCx)
@@ -234,25 +225,16 @@ SystemWorkerManager::RegisterNfcWorker(J
     return NS_ERROR_FAILURE;
   }
 
   return NfcConsumer::Register(wctd);
 #endif // MOZ_NFC
 }
 
 nsresult
-SystemWorkerManager::InitNetd(JSContext *cx)
-{
-  nsCOMPtr<nsIWorkerHolder> 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)
 {
   nsCOMPtr<nsIWorkerHolder> worker = do_CreateInstance(kWifiWorkerCID);
   NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
 
   mWifiWorker = worker;
   return NS_OK;
 }
--- a/dom/system/gonk/SystemWorkerManager.h
+++ b/dom/system/gonk/SystemWorkerManager.h
@@ -54,21 +54,19 @@ public:
 
   static nsIInterfaceRequestor*
   GetInterfaceRequestor();
 
 private:
   SystemWorkerManager();
   ~SystemWorkerManager();
 
-  nsresult InitNetd(JSContext *cx);
   nsresult InitWifi(JSContext *cx);
   nsresult InitKeyStore(JSContext *cx);
 
-  nsCOMPtr<nsIWorkerHolder> mNetdWorker;
   nsCOMPtr<nsIWorkerHolder> mWifiWorker;
 
   nsRefPtr<ipc::KeyStore> mKeyStore;
 
   bool mShutdown;
 };
 
 }