Last Comment Bug 413590 - Remaining null-arg checks in TB
: Remaining null-arg checks in TB
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
Mentors:
Depends on: 417493 611233
Blocks: 412109
  Show dependency treegraph
 
Reported: 2008-01-22 19:32 PST by Joey Minta
Modified: 2012-02-24 04:29 PST (History)
10 users (show)
dmose: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (9.41 KB, patch)
2008-01-22 19:32 PST, Joey Minta
standard8: review+
Details | Diff | Review
patch v2 (29.22 KB, patch)
2012-02-10 13:32 PST, :aceman
standard8: review+
Details | Diff | Review
patch v3 (30.87 KB, patch)
2012-02-23 15:39 PST, :aceman
acelists: review+
Details | Diff | Review

Description Joey Minta 2008-01-22 19:32:06 PST
Created attachment 298640 [details] [diff] [review]
patch v1

This patch covers the remaining null-arg checks for anything with "messenger" in the contract. :-)
Comment 1 Mark Banner (:standard8) 2008-01-28 05:17:05 PST
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 2008-02-13 19:53:48 PST
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 2008-02-14 08:50:44 PST
Did this ever get sr?
Comment 4 Joey Minta 2008-02-14 09:16:50 PST
(In reply to comment #3)
I had a blanket rubber-stamp from dmose for null-checks. 

Comment 5 Joey Minta 2008-02-14 12:25:41 PST
I've backed out the patch due to the regressions.
Comment 6 Joe Sabash [:JoeS1] 2008-02-14 15:15:15 PST
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 2008-02-14 22:23:30 PST
The regression was bug 417493.
Comment 8 Mark Banner (:standard8) 2008-03-13 06:00:55 PDT
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, NI for questions) 2009-05-17 04:26:08 PDT
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, NI for questions) 2010-03-14 05:29:09 PDT
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] 2010-08-06 09:09:31 PDT
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 2012-01-11 08:07:46 PST
(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) 2012-02-03 04:14:37 PST
(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 2012-02-06 11:20:09 PST
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 2012-02-10 13:32:31 PST
Created attachment 596159 [details] [diff] [review]
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) 2012-02-23 14:58:31 PST
Oh, note there's a little bit of bitrot in the patch that needs fixing up before landing.
Comment 17 :aceman 2012-02-23 15:39:41 PST
Created attachment 600212 [details] [diff] [review]
patch v3

Done.
Comment 18 Mark Banner (:standard8) 2012-02-24 02:45:05 PST
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 2012-02-24 04:29:03 PST
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.

Note You need to log in before you can comment on or make changes to this bug.