413590 - Remaining null-arg checks in TB

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: patch v1

This patch covers the remaining null-arg checks for anything with "messenger" in the contract. :-)

Comment 1

[Mark Banner (:standard8)]

Comment on attachment 298640 [details] [diff] [review]
patch v1

+  NS_ENSURE_ARG_POINTER(charset);

nit: blank lines after these please.

+  if (!m_folders)
+    return NS_ERROR_FAILURE;
   return m_folders->QueryElementAt(index, NS_GET_IID(nsIMsgFolder), (void **) aFolder);

nit: this type of construct could also do with a blank line after the first return as well.

r=me with those fixed.

Comment 2

[Joey Minta]

Nits picked. Patch checked in.

Checking in mailnews/base/search/src/nsMsgSearchTerm.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v  <--  nsMsgSearchTerm.cpp
new revision: 1.151; previous revision: 1.150
done
Checking in mailnews/base/src/nsMsgBiffManager.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgBiffManager.cpp,v  <--  nsMsgBiffManager.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in mailnews/base/src/nsMsgGroupView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgGroupView.cpp,v  <--  nsMsgGroupView.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in mailnews/base/src/nsMsgSearchDBView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgSearchDBView.cpp,v  <--  nsMsgSearchDBView.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp,v  <--  nsMsgXFVirtualFolderDBView.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp;
/cvsroot/mozilla/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp,v  <--  nsBayesianFilter.cpp
new revision: 1.71; previous revision: 1.70
done
Checking in mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp;
/cvsroot/mozilla/mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp,v  <--  nsMsgMailViewList.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/imap/src/nsImapService.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapService.cpp,v  <--  nsImapService.cpp
new revision: 1.333; previous revision: 1.332
done

Comment 3

[Magnus Melin [:mkmelin]]

Did this ever get sr?

Comment 4

[Joey Minta]

(In reply to comment #3)
I had a blanket rubber-stamp from dmose for null-checks. 

Comment 5

[Joey Minta]

I've backed out the patch due to the regressions.

Comment 6

[Joe Sabash [:JoeS1]]

Since the checkin went out with the nightly, it would be nice to know just what regressions were involved to not confuse issues with other outstanding bugs.

Comment 7

[Magnus Melin [:mkmelin]]

The regression was bug 417493.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 298640 [details] [diff] [review]
patch v1

Given the regressions, I thought I'd take a quick look at this again.

diff -r fe50a5e15e25 mailnews/base/search/src/nsMsgSearchTerm.cpp
--- a/mailnews/base/search/src/nsMsgSearchTerm.cpp	Tue Jan 22 20:55:22 2008 -0500
+++ b/mailnews/base/search/src/nsMsgSearchTerm.cpp	Tue Jan 22 22:29:49 2008 -0500
@@ -947,6 +947,7 @@ nsresult nsMsgSearchTerm::MatchString (c
                                        PRBool *pResult)
 {
   NS_ENSURE_ARG_POINTER(pResult);
+  NS_ENSURE_ARG_POINTER(charset);
   PRBool result = PR_FALSE;
 
   nsresult err = NS_OK;

This bit shouldn't be needed, charset is explictly null-checked:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp&rev=1.153&mark=963-971#944

So if charset is really causing the crash in this function, I'd be surprised.

 NS_IMETHODIMP
 nsMsgXFVirtualFolderDBView::OnSearchDone(nsresult status)
 {
+  if (!m_viewFolder)
+    return NS_ERROR_NOT_INITIALIZED;

potentially a little risky do the check that early, but actually I think it will probably be ok, as the work done isn't significant enough to break anything if that isn't initialised (just noting in case we need to look at this again).

The rest of it looks ok, so maybe its just that first charset case that is the problem with this patch.

Comment 9

[Wayne Mery (:wsmwk)]

better error checking wanted TB3?

seem like this would be desirable, given that time is being added to the TB3 schedule. Do we want to drive in the entire suite of bug 412109? (all remaining bugs have initial reviews) Or just bits?

Comment 10

[Wayne Mery (:wsmwk)]

Gary, do patches for these - https://bugzilla.mozilla.org/buglist.cgi?bugidtype=include&bug_id=413521%2C413544%2C413548%2C413590&query_format=advanced - still apply?

Comment 11

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

Comment on attachment 298640 [details] [diff] [review]
patch v1

Time for an updated patch - though r+, it was backed out. Btw, it's now obsoleted.

$ patch -p1 --dry-run < ~/Desktop/p413590.diff 
patching file mailnews/base/search/src/nsMsgSearchTerm.cpp
Hunk #1 succeeded at 1083 (offset 136 lines).
patching file mailnews/base/src/nsMsgBiffManager.cpp
Hunk #1 FAILED at 154.
Hunk #2 FAILED at 212.
2 out of 2 hunks FAILED -- saving rejects to file mailnews/base/src/nsMsgBiffManager.cpp.rej
patching file mailnews/base/src/nsMsgGroupView.cpp
Hunk #1 FAILED at 625.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/base/src/nsMsgGroupView.cpp.rej
patching file mailnews/base/src/nsMsgSearchDBView.cpp
Hunk #1 FAILED at 129.
Hunk #2 FAILED at 169.
Hunk #3 FAILED at 253.
Hunk #4 FAILED at 522.
4 out of 4 hunks FAILED -- saving rejects to file mailnews/base/src/nsMsgSearchDBView.cpp.rej
patching file mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
Hunk #1 succeeded at 320 with fuzz 2 (offset 47 lines).
Hunk #2 FAILED at 369.
Hunk #3 FAILED at 384.
2 out of 3 hunks FAILED -- saving rejects to file mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp.rej
patching file mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
Hunk #1 succeeded at 1405 (offset 351 lines).
Hunk #2 succeeded at 1862 with fuzz 2 (offset 615 lines).
patching file mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp
patching file mailnews/imap/src/nsImapService.cpp
Hunk #1 FAILED at 826.
Hunk #2 FAILED at 1124.
Hunk #3 succeeded at 1588 (offset 122 lines).
Hunk #4 succeeded at 2823 (offset 124 lines).
2 out of 4 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapService.cpp.rej

Comment 12

[:aceman]

(In reply to Mark Banner (:standard8) from comment #8)
> diff -r fe50a5e15e25 mailnews/base/search/src/nsMsgSearchTerm.cpp
> --- a/mailnews/base/search/src/nsMsgSearchTerm.cpp	Tue Jan 22 20:55:22 2008
> -0500
> +++ b/mailnews/base/search/src/nsMsgSearchTerm.cpp	Tue Jan 22 22:29:49 2008
> -0500
> @@ -947,6 +947,7 @@ nsresult nsMsgSearchTerm::MatchString (c
>                                         PRBool *pResult)
>  {
>    NS_ENSURE_ARG_POINTER(pResult);
> +  NS_ENSURE_ARG_POINTER(charset);
>    PRBool result = PR_FALSE;
>  
>    nsresult err = NS_OK;
> 
> This bit shouldn't be needed, charset is explictly null-checked:
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/search/
> src/nsMsgSearchTerm.cpp&rev=1.153&mark=963-971#944
> 
The check at that spot is this:
if (charset != nsnull)
{
  ConvertToUnicode(charset, stringToMatch ? stringToMatch : "",
                        utf16StrToMatch);
 } else {
     NS_ASSERTION(MsgIsUTF8(nsDependentCString(stringToMatch)),
                   "stringToMatch is not UTF-8");
      CopyUTF8toUTF16(nsDependentCString(stringToMatch), utf16StrToMatch);

   }

So it does not return from the function even if charset is nsnull (I think NS_ASSERTION does not return, in standard builds) and continues execution of the function. But your new added check does return in null case (without setting the *pResult in any way). So the pResult will depend on whatever the caller initialised it. It can be true for all messages/filter or all false, or random.

Can this be the problem? Mark Banner?

Comment 13

[Mark Banner (:standard8)]

(In reply to :aceman from comment #12)
> So it does not return from the function even if charset is nsnull (I think
> NS_ASSERTION does not return, in standard builds) and continues execution of
> the function.

Yep, that's fine.

> But your new added check does return in null case (without
> setting the *pResult in any way). So the pResult will depend on whatever the
> caller initialised it. It can be true for all messages/filter or all false,
> or random.

If we're returning an error value from an xpcom based function (via nsresult), then it is the general rule that we don't need to set any return values - the error result overrides that need.

Comment 14

[:aceman]

Maybe the callers do not check the error value properly.
I just wanted to find out the cause for the regression the changes caused.
Do you have other explanation for the regression?

So I'll take out that part and see what happens.

Comment 15

[:aceman]

Attachment: patch v2

Updated patch.
I left out the charset part and also some 'if (m_folders)' checks which didn't compile and they probably aren't needed anymore, that variable is not a pointer now.

Comment 16

[Mark Banner (:standard8)]

Oh, note there's a little bit of bitrot in the patch that needs fixing up before landing.

Comment 17

[:aceman]

Attachment: patch v3

Done.

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/55b232b17c4c

I'm marking as fixed, if there's more work to do here, we should do it in follow-ups.

Comment 19

[:aceman]

Thanks.

I do not know if there is work left here. I only removed that part with charset checking that supposedly was causing a regression.

If the regression does not come up again, the patch here is probably OK. I can't say why the charset checking was needed. Whether Joey found it with some tool and the check is still needed (to prevent crash) just coded in some other way. I can't answer that. So you'd need to file this vague description.