From 5f7d72f9e83ee85b6fb1c9ca05d14c0ae43553ee Mon Sep 17 00:00:00 2001 From: "dougt%netscape.com" Date: Fri, 31 May 2002 20:40:11 +0000 Subject: [PATCH] fixes crash in js_FreeStack caused by a race condition in necko. see 139556 for the details. r=rpotts, sr=darin --- netwerk/base/src/nsFileTransport.cpp | 67 ++++++++++++++----- netwerk/base/src/nsFileTransport.h | 3 + netwerk/base/src/nsFileTransportService.cpp | 9 +++ netwerk/base/src/nsFileTransportService.h | 3 + netwerk/base/src/nsSocketTransport.cpp | 21 +++++- netwerk/base/src/nsSocketTransport.h | 4 ++ netwerk/base/src/nsSocketTransportService.cpp | 17 +++-- netwerk/base/src/nsSocketTransportService.h | 5 +- netwerk/dns/src/nsDnsService.cpp | 26 ++++++- xpcom/proxy/public/MANIFEST | 1 + xpcom/proxy/public/Makefile.in | 1 + xpcom/proxy/public/makefile.win | 1 + xpcom/proxy/public/nsProxyRelease.h | 42 +++++++++++- 13 files changed, 176 insertions(+), 24 deletions(-) diff --git a/netwerk/base/src/nsFileTransport.cpp b/netwerk/base/src/nsFileTransport.cpp index e04defa0548f..58c2ee5e418d 100644 --- a/netwerk/base/src/nsFileTransport.cpp +++ b/netwerk/base/src/nsFileTransport.cpp @@ -49,6 +49,7 @@ #include "nsReadableUtils.h" #include "nsIProxyObjectManager.h" #include "nsNetUtil.h" +#include "nsProxyRelease.h" static NS_DEFINE_CID(kProxyObjectManagerCID, NS_PROXYEVENT_MANAGER_CID); @@ -520,6 +521,10 @@ nsFileTransport::AsyncRead(nsIStreamListener *aListener, mTransferAmount = aTransferCount; mXferState = OPEN_FOR_READ; + nsIEventQueueService* eqService = mService->GetCachedEventQueueService(); + eqService->GetSpecialEventQueue(nsIEventQueueService::CURRENT_THREAD_EVENT_QUEUE, getter_AddRefs(mEventQ)); + NS_ASSERTION(mEventQ, "No Event Queue on calling thread"); + LOG(("nsFileTransport: AsyncRead [this=%x %s] mOffset=%d mTransferAmount=%d\n", this, mStreamName.get(), mOffset, mTransferAmount)); @@ -563,6 +568,10 @@ nsFileTransport::AsyncWrite(nsIStreamProvider *aProvider, mTransferAmount = aTransferCount; mXferState = OPEN_FOR_WRITE; + nsIEventQueueService* eqService = mService->GetCachedEventQueueService(); + eqService->GetSpecialEventQueue(nsIEventQueueService::CURRENT_THREAD_EVENT_QUEUE, getter_AddRefs(mEventQ)); + NS_ASSERTION(mEventQ, "No Event Queue on calling thread"); + LOG(("nsFileTransport: AsyncWrite [this=%x %s] mOffset=%d mTransferAmount=%d\n", this, mStreamName.get(), mOffset, mTransferAmount)); @@ -823,27 +832,38 @@ nsFileTransport::Process(nsIProgressEventSink *progressSink) // is reusing the stream. mXferState = CLOSING; DoClose(); - nsCOMPtr saveContext = mContext; - nsCOMPtr saveListener = mListener; - mListener = nsnull; - mContext = nsnull; // close the data source NS_IF_RELEASE(mSourceWrapper); mSourceWrapper = nsnull; if (progressSink) { - progressSink->OnStatus(this, saveContext, - NS_NET_STATUS_READ_FROM, - NS_ConvertASCIItoUCS2(mStreamName).get()); + progressSink->OnStatus(this, + mContext, + NS_NET_STATUS_READ_FROM, + NS_ConvertASCIItoUCS2(mStreamName).get()); } - if (saveListener) { - saveListener->OnStopRequest(this, saveContext, mStatus); - saveListener = 0; + if (mListener) { + mListener->OnStopRequest(this, mContext, mStatus); + mListener = 0; } - saveContext = 0; + // if we have a context, we have to ensure that it is released on the + // proper thread. + if (mContext) { + if (mEventQ) { + // see http://bugzilla.mozilla.org/show_bug.cgi?id=139556#c64 + // for the reason behind this evil reference counting. + nsISupports* doomed = mContext.get(); + NS_ADDREF(doomed); + mContext = 0; + NS_ProxyRelease(mEventQ, doomed); + } + else { + mContext = nsnull; + } + } break; } @@ -1002,16 +1022,31 @@ nsFileTransport::Process(nsIProgressEventSink *progressSink) NS_IF_RELEASE(mSinkWrapper); mSinkWrapper = nsnull; - if (mProvider) { - mProvider->OnStopRequest(this, mContext, mStatus); - mProvider = 0; - } if (progressSink) progressSink->OnStatus(this, mContext, NS_NET_STATUS_WROTE_TO, NS_ConvertASCIItoUCS2(mStreamName).get()); - mContext = 0; + if (mProvider) { + mProvider->OnStopRequest(this, mContext, mStatus); + mProvider = 0; + } + + // if we have a context, we have to ensure that it is released on the + // proper thread. + if (mContext) { + if (mEventQ) { + // see http://bugzilla.mozilla.org/show_bug.cgi?id=139556#c64 + // for the reason behind this evil reference counting. + nsISupports* doomed = mContext.get(); + NS_ADDREF(doomed); + mContext = 0; + NS_ProxyRelease(mEventQ, doomed); + } + else { + mContext = nsnull; + } + } mXferState = CLOSING; break; } diff --git a/netwerk/base/src/nsFileTransport.h b/netwerk/base/src/nsFileTransport.h index b67be33daadb..847203a7a3a6 100644 --- a/netwerk/base/src/nsFileTransport.h +++ b/netwerk/base/src/nsFileTransport.h @@ -131,6 +131,9 @@ protected: nsCOMPtr mContext; + // Queue where release of context, listener, and/or provider should occur. + nsCOMPtr mEventQ; + // mXferState is only changed by the file transport thread: XferState mXferState; diff --git a/netwerk/base/src/nsFileTransportService.cpp b/netwerk/base/src/nsFileTransportService.cpp index 97417aa474cc..9eee2112c8d0 100644 --- a/netwerk/base/src/nsFileTransportService.cpp +++ b/netwerk/base/src/nsFileTransportService.cpp @@ -122,6 +122,15 @@ nsFileTransportService::GetCachedMimeService() return mMimeService.get(); } +nsIEventQueueService* +nsFileTransportService::GetCachedEventQueueService() +{ + if (!mEventQService) { + mEventQService = do_GetService(NS_EVENTQUEUESERVICE_CONTRACTID); + } + return mEventQService.get(); +} + //////////////////////////////////////////////////////////////////////////////// NS_IMETHODIMP diff --git a/netwerk/base/src/nsFileTransportService.h b/netwerk/base/src/nsFileTransportService.h index 41baae98de22..0c2804163a40 100644 --- a/netwerk/base/src/nsFileTransportService.h +++ b/netwerk/base/src/nsFileTransportService.h @@ -42,6 +42,7 @@ #include "nsIThreadPool.h" #include "nsSupportsArray.h" #include "nsIMIMEService.h" +#include "nsIEventQueueService.h" #define NS_FILE_TRANSPORT_WORKER_COUNT_MIN 1 #define NS_FILE_TRANSPORT_WORKER_COUNT_MAX 4//16 @@ -64,6 +65,7 @@ public: static nsFileTransportService *GetInstance() { return mInstance; } nsIMIMEService* GetCachedMimeService(); + nsIEventQueueService* GetCachedEventQueueService(); PRInt32 mConnectedTransports; PRInt32 mTotalTransports; @@ -79,6 +81,7 @@ protected: nsCOMPtr mPool; PRLock* mLock; nsCOMPtr mMimeService; + nsCOMPtr mEventQService; static nsFileTransportService* mInstance; }; diff --git a/netwerk/base/src/nsSocketTransport.cpp b/netwerk/base/src/nsSocketTransport.cpp index 3e2051e33c50..0ee7de7f525a 100644 --- a/netwerk/base/src/nsSocketTransport.cpp +++ b/netwerk/base/src/nsSocketTransport.cpp @@ -61,6 +61,7 @@ #include "nsITransportSecurityInfo.h" #include "nsMemory.h" #include "nsIProxyInfo.h" +#include "nsProxyRelease.h" #if defined(PR_LOGGING) static PRLogModuleInfo *gSocketTransportLog = nsnull; @@ -2637,6 +2638,12 @@ nsSocketRequest::SetTransport(nsSocketTransport *aTransport) // NS_IF_RELEASE(mTransport); NS_IF_ADDREF(mTransport = aTransport); + + // + // Set the event queue + // + nsIEventQueueService* eqService = aTransport->mService->GetCachedEventQueueService(); + eqService->GetSpecialEventQueue(nsIEventQueueService::CURRENT_THREAD_EVENT_QUEUE, getter_AddRefs(mEventQ)); } nsresult @@ -2657,9 +2664,21 @@ nsSocketRequest::OnStop() mObserver->OnStartRequest(this, mContext); mStartFired = PR_TRUE; } + mObserver->OnStopRequest(this, mContext, mStatus); mObserver = 0; - mContext = 0; + + if (mContext) { + if (mEventQ) { + nsISupports* doomed = mContext.get(); + NS_ADDREF(doomed); + mContext = 0; + NS_ProxyRelease(mEventQ, doomed); + } + else { + mContext = 0; + } + } mStopFired = PR_TRUE; } return NS_OK; diff --git a/netwerk/base/src/nsSocketTransport.h b/netwerk/base/src/nsSocketTransport.h index 8ced37acd753..6782ac3ad3b8 100644 --- a/netwerk/base/src/nsSocketTransport.h +++ b/netwerk/base/src/nsSocketTransport.h @@ -309,6 +309,8 @@ protected: nsSocketBOS *mBOS; // weak reference nsSocketReadRequest *mReadRequest; nsSocketWriteRequest *mWriteRequest; + + friend nsSocketRequest; }; /** @@ -453,6 +455,8 @@ protected: nsSocketTransport *mTransport; nsCOMPtr mObserver; nsCOMPtr mContext; + // Queue where release of context, listener, and/or provider should occur. + nsCOMPtr mEventQ; nsresult mStatus; PRIntn mSuspendCount; PRPackedBool mCanceled; diff --git a/netwerk/base/src/nsSocketTransportService.cpp b/netwerk/base/src/nsSocketTransportService.cpp index 932d03bd00a2..100304edada1 100644 --- a/netwerk/base/src/nsSocketTransportService.cpp +++ b/netwerk/base/src/nsSocketTransportService.cpp @@ -211,6 +211,15 @@ nsSocketTransportService::Init(void) rv = NS_ERROR_UNEXPECTED; } } + + + if (NS_SUCCEEDED(rv) && !mEventQService) { + mEventQService = do_GetService(NS_EVENTQUEUESERVICE_CONTRACTID); + if (!mEventQService) { + rv = NS_ERROR_UNEXPECTED; + } + } + return rv; } @@ -791,22 +800,22 @@ nsSocketTransportService::GetNeckoStringByName (const char *aName, PRUnichar **a nsresult res; nsAutoString resultString; resultString.AssignWithConversion(aName); - if (!m_stringBundle) { + if (!mStringBundle) { const char propertyURL[] = NECKO_MSGS_URL; // make sure that we get this service on the UI thread. NS_WITH_PROXIED_SERVICE(nsIStringBundleService, sBundleService, kStringBundleServiceCID, NS_UI_THREAD_EVENTQ, &res); if (NS_SUCCEEDED (res) && (nsnull != sBundleService)) { - res = sBundleService->CreateBundle(propertyURL, getter_AddRefs(m_stringBundle)); + res = sBundleService->CreateBundle(propertyURL, getter_AddRefs(mStringBundle)); } } - if (m_stringBundle) + if (mStringBundle) { nsAutoString unicodeName; unicodeName.AssignWithConversion(aName); PRUnichar *ptrv = nsnull; - res = m_stringBundle->GetStringFromName(unicodeName.get(), &ptrv); + res = mStringBundle->GetStringFromName(unicodeName.get(), &ptrv); if (NS_FAILED(res)) { diff --git a/netwerk/base/src/nsSocketTransportService.h b/netwerk/base/src/nsSocketTransportService.h index 2cb8418a72bc..66e365fd23de 100644 --- a/netwerk/base/src/nsSocketTransportService.h +++ b/netwerk/base/src/nsSocketTransportService.h @@ -46,6 +46,7 @@ #include "nsCOMPtr.h" #include "nsIStringBundle.h" #include "nsIDNSService.h" +#include "nsIEventQueueService.h" #if defined(XP_PC) || defined(XP_UNIX) || defined(XP_BEOS) || defined(XP_MAC) // @@ -99,6 +100,7 @@ public: nsresult GetNeckoStringByName (const char *aName, PRUnichar **aString); nsIDNSService* GetCachedDNSService() { return mDNSService.get(); } + nsIEventQueueService* GetCachedEventQueueService() { return mEventQService.get(); } protected: nsIThread* mThread; @@ -111,8 +113,9 @@ protected: PRInt32 mSelectFDSetCount; PRPollDesc* mSelectFDSet; nsSocketTransport** mActiveTransportList; - nsCOMPtr m_stringBundle; + nsCOMPtr mStringBundle; nsCOMPtr mDNSService; + nsCOMPtr mEventQService; }; diff --git a/netwerk/dns/src/nsDnsService.cpp b/netwerk/dns/src/nsDnsService.cpp index c38c68fca84b..2d26869fe734 100644 --- a/netwerk/dns/src/nsDnsService.cpp +++ b/netwerk/dns/src/nsDnsService.cpp @@ -1026,6 +1026,15 @@ nsDNSService::Init() rv = InstallPrefObserver(); if (NS_FAILED(rv)) return rv; + // install xpcom shutdown observer + nsCOMPtr observerService = + do_GetService("@mozilla.org/observer-service;1", &rv); + if (NS_FAILED(rv)) return rv; + + rv = observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_FALSE); + if (NS_FAILED(rv)) return rv; + + mState = DNS_ONLINE; return NS_OK; @@ -1265,6 +1274,13 @@ nsDNSService::Observe(nsISupports * subject, { nsresult rv = NS_OK; + if (!nsCRT::strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, topic)) + { + // we need to shutdown! + ShutdownInternal(); + return NS_OK; + } + if (nsCRT::strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, topic)) return NS_OK; @@ -1302,7 +1318,6 @@ nsDNSService::Observe(nsISupports * subject, mIDNConverter = nsnull; } } - return rv; } @@ -1829,6 +1844,15 @@ nsDNSService::ShutdownInternal() (void) RemovePrefObserver(); + + // remove xpcom shutdown observer + nsCOMPtr observerService = + do_GetService("@mozilla.org/observer-service;1", &rv); + if (NS_FAILED(rv)) return rv; + + rv = observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID); + if (NS_FAILED(rv)) return rv; + // reset hashtable // XXX assert hashtable is empty PL_DHashTableFinish(&mHashTable); diff --git a/xpcom/proxy/public/MANIFEST b/xpcom/proxy/public/MANIFEST index 9b7909a54fc3..5625683fe1e7 100644 --- a/xpcom/proxy/public/MANIFEST +++ b/xpcom/proxy/public/MANIFEST @@ -3,4 +3,5 @@ # nsProxyEvent.h +nsProxyRelease.h nsProxiedService.h diff --git a/xpcom/proxy/public/Makefile.in b/xpcom/proxy/public/Makefile.in index 3045ebc846c1..306941e0b240 100644 --- a/xpcom/proxy/public/Makefile.in +++ b/xpcom/proxy/public/Makefile.in @@ -35,6 +35,7 @@ endif EXPORTS = \ nsProxyEvent.h \ + nsProxyRelease.h \ nsProxiedService.h \ $(NULL) diff --git a/xpcom/proxy/public/makefile.win b/xpcom/proxy/public/makefile.win index 57868c647cfa..f12cc0dba563 100644 --- a/xpcom/proxy/public/makefile.win +++ b/xpcom/proxy/public/makefile.win @@ -25,6 +25,7 @@ MODULE=xpcom EXPORTS = \ nsProxyEvent.h \ + nsProxyRelease.h \ nsProxiedService.h \ $(NULL) diff --git a/xpcom/proxy/public/nsProxyRelease.h b/xpcom/proxy/public/nsProxyRelease.h index 0fa97f0ab6bb..0f8e779d340f 100644 --- a/xpcom/proxy/public/nsProxyRelease.h +++ b/xpcom/proxy/public/nsProxyRelease.h @@ -50,7 +50,7 @@ static void* PR_CALLBACK ReleaseDestructorEventHandler(PLEvent *self) { nsISupports* owner = (nsISupports*) PL_GetEventOwner(self); - NS_DELETEXPCOM(owner); + NS_RELEASE(owner); return nsnull; } @@ -60,6 +60,44 @@ ReleaseDestructorDestroyHandler(PLEvent *self) PR_DELETE(self); } +static void +NS_ProxyRelease(nsIEventQueue *eventQ, nsISupports *doomed, PRBool alwaysProxy=PR_FALSE) +{ + if (!doomed) + return; + + if (!eventQ) { + NS_RELEASE(doomed); + return; + } + + if (!alwaysProxy) { + PRBool onCurrentThread = PR_FALSE; + eventQ->IsQueueOnCurrentThread(&onCurrentThread); + if (onCurrentThread) { + NS_RELEASE(doomed); + return; + } + } + + PLEvent *ev = new PLEvent; + if (!ev) { + NS_ERROR("failed to allocate PLEvent"); + // we do not release doomed here since it may cause a delete on the the + // wrong thread. better to leak than crash. + return; + } + + PL_InitEvent(ev, + (void *) doomed, + ReleaseDestructorEventHandler, + ReleaseDestructorDestroyHandler); + + PRStatus rv = eventQ->PostEvent(ev); + NS_ASSERTION(rv == PR_SUCCESS, "PostEvent failed"); +} + + #define NS_IMPL_PROXY_RELEASE(_class) \ NS_IMETHODIMP_(nsrefcnt) _class::Release(void) \ { \ @@ -106,5 +144,7 @@ NS_IMETHODIMP_(nsrefcnt) _class::Release(void) return count; \ } \ + + #endif