Last Comment Bug 413521 - add null-arg checks in nsMsgFilterService.cpp, nsMsgFolderCompactor.cpp, and nsMsgPrintEngine.cpp
: add null-arg checks in nsMsgFilterService.cpp, nsMsgFolderCompactor.cpp, and ...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 11.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 712312
Blocks: 412109
  Show dependency treegraph
 
Reported: 2008-01-22 11:43 PST by Joey Minta
Modified: 2016-02-21 05:38 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (4.13 KB, patch)
2008-01-22 11:43 PST, Joey Minta
standard8: review-
Details | Diff | Splinter Review
patch for PrintEngine and FolderCompactor (4.15 KB, patch)
2011-11-15 12:00 PST, :aceman
standard8: review-
Details | Diff | Splinter Review
2nd try, patch for PrintEngine and FolderCompactor (4.29 KB, patch)
2011-11-24 13:50 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
final patch for PrintEngine and FolderCompactor [check this in] (4.23 KB, patch)
2011-12-02 12:57 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
patch for nsMsgFilterService.cpp (11.14 KB, patch)
2011-12-02 12:59 PST, :aceman
no flags Details | Diff | Splinter Review
v2, patch for nsMsgFilterService.cpp (10.93 KB, patch)
2011-12-05 14:09 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
final patch for nsMsgFilterService.cpp [check this in too] (12.09 KB, patch)
2011-12-19 10:24 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Joey Minta 2008-01-22 11:43:02 PST
Created attachment 298516 [details] [diff] [review]
patch v1

These should all be pretty self-explanatory.  I also did whitespace cleanup for nsMsgFilterService::OpenFilterList, since it was pretty ugly.
Comment 1 Mark Banner (:standard8, afk until Dec) 2008-01-28 05:09:02 PST
Comment on attachment 298516 [details] [diff] [review]
patch v1

+  if (!m_folder)
+    return NS_ERROR_NOT_INITIALIZED;

nit: please put this above the function header.

-   FinishCompact();
+    rv = FinishCompact();
+    NS_ENSURE_SUCCESS(rv, rv);
     Release(); // kill self

This looks like a bad idea. Firstly, we'll leak if FinishCompact fails, secondly, I'm not sure if OnStopRequest will propogate error cases up to the UI/the original caller.

Perhaps we could assert, but still allow the Release() to go ahead.

Bonus points for killing the goto in OnStopRequest in the next version ;-)
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2010-08-06 08:29:44 PDT
Comment on attachment 298516 [details] [diff] [review]
patch v1

Patch is now obsolete (along with r-).

$ patch -p1 --dry-run < ~/Desktop/p413521.diff 
patching file mailnews/base/search/src/nsMsgFilterService.cpp
Hunk #1 succeeded at 88 (offset 10 lines).
Hunk #2 succeeded at 110 (offset 10 lines).
Hunk #3 succeeded at 124 (offset 10 lines).
Hunk #4 succeeded at 826 (offset 37 lines).
patching file mailnews/base/src/nsMsgFolderCompactor.cpp
Hunk #1 succeeded at 400 (offset 36 lines).
Hunk #2 FAILED at 591.
1 out of 2 hunks FAILED -- saving rejects to file mailnews/base/src/nsMsgFolderCompactor.cpp.rej
patching file mailnews/base/src/nsMsgPrintEngine.cpp
Hunk #1 succeeded at 331 (offset -4 lines).
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2010-12-07 05:51:48 PST
serge, does this get picked up in one of your other bugs?
I am particularly interested in seeing the compact and filter items land.
Comment 4 Serge Gautherie (:sgautherie) 2010-12-07 06:30:20 PST
(In reply to comment #3)
> serge, does this get picked up in one of your other bugs?

Not yet: I started to work on bug 611233 and I'm waiting for review :-|
Comment 5 Wayne Mery (:wsmwk, NI for questions) 2011-05-31 06:32:44 PDT
bienvenu, can you drive in the couple lines for nsMsgFolderCompactor.cpp?
Comment 6 David :Bienvenu 2011-05-31 07:27:26 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > serge, does this get picked up in one of your other bugs?
> 
> Not yet: I started to work on bug 611233 and I'm waiting for review :-|

I believe you got a review. And you were doing the opposite of this bug, removing null arg checks, some of which were required by xpcom; this bug is about adding null arg checks.
Comment 7 :aceman 2011-11-14 08:08:49 PST
Serge, Joey, will you continue with this patch?
Or should I pick it up?
Comment 8 :aceman 2011-11-15 00:19:47 PST
Thanks :)
Does anybody know why this is marked 'crash' ? Are there any known crashes from this? Or does it mean it only crashes in the testsuite referenced in bug 412109?
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2011-11-15 04:09:58 PST
Seems reasonable
Comment 10 :aceman 2011-11-15 12:00:21 PST
Created attachment 574643 [details] [diff] [review]
patch for PrintEngine and FolderCompactor

I hope I addressed comment 1.

I'll do FilterService in a separate patch as I have other unrelated changes pending there.
Comment 11 Mark Banner (:standard8, afk until Dec) 2011-11-24 04:41:43 PST
Comment on attachment 574643 [details] [diff] [review]
patch for PrintEngine and FolderCompactor

Review of attachment 574643 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +609,2 @@
>  NS_IMETHODIMP
>  nsFolderCompactState::OnStopRequest(nsIRequest *request, nsISupports *ctxt,

The way the changes are structured in this function, you'll end up with cases where you can get multiple Release calls.

I think the better way to go would be something like:

if (NS_FAILED(status))
{
  // Do what used to be done in the If statement of the "done:" section.
  return status;
}

EndCopy(nsnull, status);
if (m_curIndex >= m_size)
{
  // etc

I think you'll also find there's no need for the intermediate rv variable.

@@ +628,5 @@
> +    else
> +    {
> +      // in case we're not getting an error, we still need to pretend we did get an error,
> +      // because the compact did not successfully complete.
> +      if (NS_SUCCEEDED(status))

This if comes straight after an else, so you can just make it "else if".
Comment 12 :aceman 2011-11-24 11:45:50 PST
(In reply to Mark Banner (:standard8) from comment #11)
> if (NS_FAILED(status))
> {
>   // Do what used to be done in the If statement of the "done:" section.
>   return status;
> }
> 
> EndCopy(nsnull, status);
> if (m_curIndex >= m_size)
> {
>   // etc
> 
> I think you'll also find there's no need for the intermediate rv variable.
> 
> @@ +628,5 @@
> > +    else
> > +    {
> > +      // in case we're not getting an error, we still need to pretend we did get an error,
> > +      // because the compact did not successfully complete.
> > +      if (NS_SUCCEEDED(status))
> 
> This if comes straight after an else, so you can just make it "else if".

Actually, why does it check for NS_SUCCEEDED(status), when we already checked NS_FAILED(rv=status)? Is that check needed? Did status change somewhere? I don't see it can change in EndCopy.
Comment 13 :aceman 2011-11-24 13:50:35 PST
Created attachment 576824 [details] [diff] [review]
2nd try, patch for PrintEngine and FolderCompactor

What about this?
Comment 14 Mark Banner (:standard8, afk until Dec) 2011-12-02 04:17:55 PST
Comment on attachment 576824 [details] [diff] [review]
2nd try, patch for PrintEngine and FolderCompactor

Review of attachment 576824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks better.

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +609,5 @@
>  NS_IMETHODIMP
>  nsFolderCompactState::OnStopRequest(nsIRequest *request, nsISupports *ctxt,
>                                      nsresult status)
>  {
> +//  nsresult rv = status;

nit: Just remove this completely.

@@ +629,5 @@
> +      newMsgHdr = nsnull;
> +      // no more to copy finish it up
> +      FinishCompact();
> +    }
> +    else //if (NS_SUCCEEDED(status))

I think we can remove this comment as well.

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ +332,5 @@
>  NS_IMETHODIMP
>  nsMsgPrintEngine::AddPrintURI(const PRUnichar *aMsgURI)
>  {
> +  NS_ENSURE_ARG_POINTER(aMsgURI);
> +

nit: no need for blank line here.
Comment 15 :aceman 2011-12-02 05:26:56 PST
Thanks. Yes, I left the commented out code as a hint what I suggested to remove. As you agree with it, I'll of course remove it.

(In reply to Mark Banner (:standard8) from comment #14)
> ::: mailnews/base/src/nsMsgPrintEngine.cpp
> @@ +332,5 @@
> >  NS_IMETHODIMP
> >  nsMsgPrintEngine::AddPrintURI(const PRUnichar *aMsgURI)
> >  {
> > +  NS_ENSURE_ARG_POINTER(aMsgURI);
> > +
> 
> nit: no need for blank line here.

nit: In the past, you wanted blank lines here (see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=413544#c1). Has the preferred style changed?
Comment 16 Mark Banner (:standard8, afk until Dec) 2011-12-02 06:03:07 PST
(In reply to :aceman from comment #15)
> nit: In the past, you wanted blank lines here (see e.g.
> https://bugzilla.mozilla.org/show_bug.cgi?id=413544#c1). Has the preferred
> style changed?

Heh, keep it in then.
Comment 17 :aceman 2011-12-02 12:57:56 PST
Created attachment 578687 [details] [diff] [review]
final patch for PrintEngine and FolderCompactor [check this in]

Carrying over r+mbanner from comment 14.
Comment 18 :aceman 2011-12-02 12:59:25 PST
Created attachment 578688 [details] [diff] [review]
patch for nsMsgFilterService.cpp
Comment 19 :aceman 2011-12-04 13:43:54 PST
--- a/mailnews/base/search/src/nsMsgFilterService.cpp	Tue Jan 22 14:11:52 2008 -0500
+++ b/mailnews/base/search/src/nsMsgFilterService.cpp	Tue Jan 22 14:40:46 2008 -0500
@@ -78,18 +78,20 @@ nsMsgFilterService::~nsMsgFilterService(
 
 NS_IMETHODIMP nsMsgFilterService::OpenFilterList(nsILocalFile *aFilterFile, nsIMsgFolder *rootFolder, nsIMsgWindow *aMsgWindow, nsIMsgFilterList **resultFilterList)
 {

+  NS_ENSURE_ARG_POINTER(rootFolder);

It seems this line from the original patch causes test failure in imap/test/unit/test_trustSpamAssassin.js . Should I remove that line, or could the test be bad?
Comment 20 David :Bienvenu 2011-12-05 08:18:34 PST
in parameters are allowed to be null. Is there some reason we think rootFolder isn't allowed to be null.
Comment 21 :aceman 2011-12-05 08:35:32 PST
I don't know. It was in the original patch from joey. The conditions may have changed since then. That's why I ask. So I will remove this line in my version.
Comment 22 :aceman 2011-12-05 14:09:10 PST
Created attachment 579159 [details] [diff] [review]
v2, patch for nsMsgFilterService.cpp
Comment 23 Mark Banner (:standard8, afk until Dec) 2011-12-19 05:10:26 PST
Comment on attachment 579159 [details] [diff] [review]
v2, patch for nsMsgFilterService.cpp

Review of attachment 579159 [details] [diff] [review]:
-----------------------------------------------------------------

One minor comment, r=Standard8 with that fixed.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +435,5 @@
>  
>    if (continueExecution)
>      return m_searchHits.IsEmpty() ? RunNextFilter() : ApplyFilter();
>    else
> +    return OnEndExecution(status);

nit, because there's a return just before the else, please drop the else and have the return on its own.
Comment 24 :aceman 2011-12-19 10:24:35 PST
Created attachment 582875 [details] [diff] [review]
final patch for nsMsgFilterService.cpp [check this in too]

Carrying over r=Standard8.
Comment 26 Mike Conley (:mconley) 2011-12-20 08:04:33 PST
This patch has broken Troubleshooting Information, and is causing test failures (see bug 712312).

This appears to be due to the change in aboutSupport.xhtml (see https://bugzilla.mozilla.org/attachment.cgi?id=582875&action=diff#a/mail/components/about-support/content/aboutSupport.xhtml_sec1).

Why has this change been made?
Comment 27 :aceman 2011-12-20 08:14:11 PST
Because I am making my way through learning to use mq in hg ...

Sorry about that :(

That change aboutSupport.xhtml does not belong into this bug and should not have been merged. (It is for bug 559500.)

Can you please fix it or back out?
Comment 28 :aceman 2011-12-20 08:37:38 PST
Fixed in bug 712312, thanks to Mike.

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