413578 - null-arg checks in nsMsgAccountManager.cpp, nsMsgPurgeService.cpp, nsMsgIdentity.cpp, and nsImapIncomingServer.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: patch v1

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 1

[Mark Banner (:standard8)]

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.

Comment 2

[Wayne Mery (:wsmwk)]

this should be easy to drive in?

Comment 3

[Wayne Mery (:wsmwk)]

gary, how much of this still applies?

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> gary, how much of this still applies?

Mostly. A sort-of unbitrotted patch is coming up. :)

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2

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

Comment 6

[Mark Banner (:standard8)]

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 7

[David :Bienvenu]

Comment on attachment 534697 [details] [diff] [review]
patch v2

thx for unbitrotting this, Gary.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Fixed whitespace issues as well.

http://hg.mozilla.org/comm-central/rev/119c61c869a8