Turns out that I wasn't finished with nsImapIncomingServer in the last patch. This one also includes 2 compiler warning fixes in nsMsgAccount.cpp for unused variables. nsMsgIdentity crashed in almost every function here (more mPrefBranch uninitialization) and is so simple/self-contained that I was really tempted to just stop fixing it and rewrite it in js. :-/ Nonetheless, here's the patch. Note that my current statistics say we're about 3/4 done at this point.

Comment on attachment 298618 [details] [diff] [review] patch v1 firstly, just for the review record: nsMsgAccountManager::WriteToFolderCache(nsIMsgFolderCache *folderCache) { m_incomingServers.Enumerate(hashWriteFolderCache, folderCache); - return folderCache->Close(); + return folderCache ? folderCache->Close() : NS_ERROR_FAILURE; the rest of the code seems to treat folderCache in this manner, so this appears to be ok. Now for the main comments: + NS_ENSURE_ARG_POINTER(server); PRInt32 count = m_incomingServerListeners.Count(); + if (!mPrefBranch) + return NS_ERROR_NOT_INITIALIZED; nsresult rv = mPrefBranch->GetCharPref(prefname, getter_Copies(retval)); nit: these need a blank line between what you've added and the next bit. @@ -3215,7 +3217,10 @@ nsImapIncomingServer::GetPrefForServerAt // Time to check if this server has the pref // (mail.server.<serverkey>.prefSuffix) already set - rv = mPrefBranch->GetBoolPref(prefName.get(), prefValue); + if (mPrefBranch) + rv = mPrefBranch->GetBoolPref(prefName.get(), prefValue); + else + rv = NS_ERROR_NOT_INITIALIZED; I'd much rather we just returned at the start of the function avoiding setting everything up, than doing it late. Additionally, I notice that nsCOMPtr<nsIPrefBranch> prefBranch can actually be moved inside the if (NS_FAILED(rv)), so please could you move that as well. With that change you can then do: nsresult rv = mPrefBranch->GetBoolPref(... on the one line, hence tidying that up as well.

this should be easy to drive in?

gary, how much of this still applies?

(In reply to comment #3) > gary, how much of this still applies? Mostly. A sort-of unbitrotted patch is coming up. :)

Unbitrotted patch with comments hopefully addressed, if I interpreted correctly.

Comment on attachment 534697 [details] [diff] [review] patch v2 Review of attachment 534697 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsMsgAccountManager.cpp @@ +1622,5 @@ > NS_IMETHODIMP > nsMsgAccountManager::WriteToFolderCache(nsIMsgFolderCache *folderCache) > { > m_incomingServers.Enumerate(hashWriteFolderCache, folderCache); > + return folderCache ? folderCache->Close() : NS_ERROR_FAILURE; From the looks of it, this function should just return a null arg if folderCache isn't supplied. Going through the enumerate it seems like nothing actually gets done, even though there's a bunch of sub-folder iteration. I'd check with David to make sure though. ::: mailnews/base/src/nsMsgPurgeService.cpp @@ -506,3 +506,3 @@ > > } > > > > NS_IMETHODIMP nsMsgPurgeService::OnSearchDone(nsresult status) In this function, you're missing indenting of the lines where

Comment on attachment 534697 [details] [diff] [review] patch v2 thx for unbitrotting this, Gary.

Fixed whitespace issues as well. http://hg.mozilla.org/comm-central/rev/119c61c869a8