Bug 977995 - remove mNetdWorker from SystemWorkerManager. r=khuey, f=vchang
authorVicamo Yang <vyang@mozilla.com>
Fri, 11 Apr 2014 22:27:55 +0800
changeset 178043 9df1bf74a0c6a61f24fa4c1748c414cda30a012e
parent 178042 9778da56668bcae44daf89856bffccc21b67d17a
child 178044 8e6b5a013750f289c43fe657c7987c6b52377082
push id6149
push uservyang@mozilla.com
push dateFri, 11 Apr 2014 14:28:18 +0000
treeherderb2g-inbound@2ad672e5ba8d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs977995
milestone31.0a1
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;
 };
 
 }