Remaining null-arg checks in TB

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Joey Minta, Assigned: aceman)

Tracking

(Blocks: 1 bug, {crash})

Trunk
Thunderbird 13.0
crash
Dependency tree / graph
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

30.87 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Created attachment 298640 [details] [diff] [review]
patch v1

This patch covers the remaining null-arg checks for anything with "messenger" in the contract. :-)
Attachment #298640 - Flags: superreview?(dmose)
Attachment #298640 - Flags: review?(bugzilla)
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.
Attachment #298640 - Flags: review?(bugzilla) → review+
(Reporter)

Comment 2

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 417493

Comment 3

10 years ago
Did this ever get sr?
(Reporter)

Comment 4

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

(Reporter)

Comment 5

10 years ago
I've backed out the patch due to the regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 6

10 years ago
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

10 years ago
The regression was bug 417493.
(Reporter)

Updated

9 years ago
Attachment #298640 - Flags: superreview?(dmose)
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.
No longer blocks: 417493
Depends on: 417493
Product: Core → MailNews Core
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?
Flags: wanted-thunderbird3?

Updated

8 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Gary, do patches for these - https://bugzilla.mozilla.org/buglist.cgi?bugidtype=include&bug_id=413521%2C413544%2C413548%2C413590&query_format=advanced - still apply?
Keywords: helpwanted
Whiteboard: [has draft patch][patchlove]
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
Attachment #298640 - Attachment is obsolete: true
Depends on: 611233
(Assignee)

Comment 12

6 years ago
(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?
(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.
(Assignee)

Comment 14

5 years ago
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.
Assignee: jminta → acelists
(Assignee)

Comment 15

5 years ago
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.
Attachment #596159 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED
Attachment #596159 - Flags: review?(mbanner) → review+
Oh, note there's a little bit of bitrot in the patch that needs fixing up before landing.
(Assignee)

Comment 17

5 years ago
Created attachment 600212 [details] [diff] [review]
patch v3

Done.
Attachment #596159 - Attachment is obsolete: true
Attachment #600212 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: helpwanted → checkin-needed
Whiteboard: [has draft patch][patchlove]
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Comment 19

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.