From 1868bf16e57c2db5d5762b9af1678640e1d192ac Mon Sep 17 00:00:00 2001 From: "dwitte%stanford.edu" Date: Tue, 24 Jun 2003 05:00:22 +0000 Subject: [PATCH] Fixing leak regression from bug 200632 landing (the latest cookie rewrite). This changes the nsCookieService singleton fu to AddRef only once, rather than twice (and removes the corresponding Release in the module dtor). The double-addref was making the cookie module stick around until XPCOM starts shutting down modules, which is too late to release things we own (observer service, prefbranch, etc). So we were violating XPCOM shutdown rules without knowing it. So, the tracemalloc leak stats on Brad should halve. :) b=209571, r=alecf, sr=darin. git-svn-id: svn://10.0.0.236/trunk@144084 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/extensions/cookie/nsCookieService.cpp | 80 ++++++++----------- mozilla/extensions/cookie/nsCookieService.h | 2 - mozilla/extensions/cookie/nsModuleFactory.cpp | 9 +-- 3 files changed, 35 insertions(+), 56 deletions(-) diff --git a/mozilla/extensions/cookie/nsCookieService.cpp b/mozilla/extensions/cookie/nsCookieService.cpp index fc13ec69db1..48c9a1f5b50 100644 --- a/mozilla/extensions/cookie/nsCookieService.cpp +++ b/mozilla/extensions/cookie/nsCookieService.cpp @@ -280,28 +280,20 @@ nsCookieService *nsCookieService::gCookieService = nsnull; nsCookieService* nsCookieService::GetSingleton() { - if (!gCookieService) { - // create a new singleton nsCookieService - gCookieService = new nsCookieService(); - if (gCookieService) { - NS_ADDREF(gCookieService); - // init it - nsresult rv = gCookieService->Init(); - // if Init() failed, release & return - if (NS_FAILED(rv)) { - NS_RELEASE(gCookieService); - gCookieService = nsnull; - } - } + if (gCookieService) { + NS_ADDREF(gCookieService); + return gCookieService; } - NS_IF_ADDREF(gCookieService); - return gCookieService; -} -void -nsCookieService::FreeSingleton() -{ - NS_IF_RELEASE(gCookieService); + // Create a new singleton nsCookieService (note: the ctor AddRefs for us). + // We AddRef only once since XPCOM has rules about the ordering of module + // teardowns - by the time our module destructor is called, it's too late to + // Release our members (e.g. nsIObserverService and nsIPrefBranch), since GC + // cycles have already been completed and would result in serious leaks. + // See bug 209571. + gCookieService = new nsCookieService(); + + return gCookieService; } /****************************************************************************** @@ -323,33 +315,21 @@ nsCookieService::nsCookieService() , mCookieChanged(PR_FALSE) , mCookieIconVisible(PR_FALSE) { -} - -nsCookieService::~nsCookieService() -{ - if (mWriteTimer) - mWriteTimer->Cancel(); - - // clean up memory - RemoveAllFromMemory(); -} - -nsresult nsCookieService::Init() -{ - nsresult rv; + // AddRef now, so we don't cross XPCOM boundaries with a zero refcount + NS_ADDREF_THIS(); // cache and init the preferences observer InitPrefObservers(); // cache mCookieFile - rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mCookieFile)); - if (NS_SUCCEEDED(rv)) { - rv = mCookieFile->AppendNative(NS_LITERAL_CSTRING(kCookieFileName)); + NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mCookieFile)); + if (mCookieFile) { + mCookieFile->AppendNative(NS_LITERAL_CSTRING(kCookieFileName)); } Read(); - mObserverService = do_GetService("@mozilla.org/observer-service;1", &rv); + mObserverService = do_GetService("@mozilla.org/observer-service;1"); if (mObserverService) { mObserverService->AddObserver(this, "profile-before-change", PR_TRUE); mObserverService->AddObserver(this, "profile-do-change", PR_TRUE); @@ -360,18 +340,26 @@ nsresult nsCookieService::Init() mPermissionService = do_GetService(NS_COOKIEPERMISSION_CONTRACTID); // Register as an observer for the document loader - nsCOMPtr docLoaderService = do_GetService(kDocLoaderServiceCID, &rv); - if (NS_SUCCEEDED(rv)) { - nsCOMPtr progress(do_QueryInterface(docLoaderService)); - if (progress) - progress->AddProgressListener((nsIWebProgressListener*)this, - nsIWebProgress::NOTIFY_STATE_DOCUMENT | - nsIWebProgress::NOTIFY_STATE_NETWORK); + nsCOMPtr docLoaderService = do_GetService(kDocLoaderServiceCID); + nsCOMPtr progress = do_QueryInterface(docLoaderService); + if (progress) { + progress->AddProgressListener(this, + nsIWebProgress::NOTIFY_STATE_DOCUMENT | + nsIWebProgress::NOTIFY_STATE_NETWORK); } else { NS_ERROR("Couldn't get nsIDocumentLoader"); } +} - return NS_OK; +nsCookieService::~nsCookieService() +{ + gCookieService = nsnull; + + if (mWriteTimer) + mWriteTimer->Cancel(); + + // clean up memory + RemoveAllFromMemory(); } NS_IMETHODIMP diff --git a/mozilla/extensions/cookie/nsCookieService.h b/mozilla/extensions/cookie/nsCookieService.h index 7f497b1585d..b6a3dc658bd 100644 --- a/mozilla/extensions/cookie/nsCookieService.h +++ b/mozilla/extensions/cookie/nsCookieService.h @@ -85,9 +85,7 @@ class nsCookieService : public nsICookieService nsCookieService(); virtual ~nsCookieService(); - nsresult Init(); static nsCookieService* GetSingleton(); - static void FreeSingleton(); protected: void InitPrefObservers(); diff --git a/mozilla/extensions/cookie/nsModuleFactory.cpp b/mozilla/extensions/cookie/nsModuleFactory.cpp index 3702d2a3a72..f4d7398a57a 100644 --- a/mozilla/extensions/cookie/nsModuleFactory.cpp +++ b/mozilla/extensions/cookie/nsModuleFactory.cpp @@ -139,11 +139,4 @@ static const nsModuleComponentInfo components[] = { }, }; -PR_STATIC_CALLBACK(void) -cookieModuleDtor(nsIModule *aSelf) -{ - // Release our singletons - nsCookieService::FreeSingleton(); -} - -NS_IMPL_NSGETMODULE_WITH_DTOR(nsCookieModule, components, cookieModuleDtor) +NS_IMPL_NSGETMODULE(nsCookieModule, components)