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.
This commit is contained in:
Vicamo Yang 2014-04-11 22:27:55 +08:00
parent 29fdeff417
commit c969a1e2c0
5 changed files with 53 additions and 55 deletions

View File

@ -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;
}
},

View File

@ -19,6 +19,8 @@ using namespace mozilla::ipc;
namespace mozilla {
nsCOMPtr<nsIThread> gWorkerThread;
// The singleton network worker, to be used on the main thread.
StaticRefPtr<NetworkWorker> 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<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()
@ -117,6 +145,7 @@ NetworkWorker::NetworkWorker()
NetworkWorker::~NetworkWorker()
{
MOZ_ASSERT(!gNetworkWorker);
MOZ_ASSERT(!mListener);
}
already_AddRefed<NetworkWorker>
@ -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<JS::Value> aOptions, JSContext* aCx)
// 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;
}
@ -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<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);
}

View File

@ -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<NetworkWorker> 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<nsIThread> mWorkerThread;
nsCOMPtr<nsINetworkEventListener> mListener;
};

View File

@ -94,8 +94,6 @@ SystemWorkerManager::Init()
InitAutoMounter();
InitializeTimeZoneSettingObserver();
rv = InitNetd(cx);
NS_ENSURE_SUCCESS(rv, rv);
nsCOMPtr<nsIAudioManager> audioManager =
do_GetService(NS_AUDIOMANAGER_CONTRACTID);
@ -131,8 +129,6 @@ SystemWorkerManager::Shutdown()
NfcConsumer::Shutdown();
#endif
mNetdWorker = nullptr;
nsCOMPtr<nsIWifi> wifi(do_QueryInterface(mWifiWorker));
if (wifi) {
wifi->Shutdown();
@ -184,11 +180,6 @@ SystemWorkerManager::GetInterface(const nsIID &aIID, void **aResult)
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;
}
@ -238,15 +229,6 @@ SystemWorkerManager::RegisterNfcWorker(JS::Handle<JS::Value> aWorker,
#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)
{

View File

@ -59,11 +59,9 @@ 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;