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 User image 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 User image Mark Banner (:standard8) 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image :aceman 2011-11-14 08:08:49 PST
Serge, Joey, will you continue with this patch?
Or should I pick it up?
Comment 8 User image :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 User image Wayne Mery (:wsmwk, NI for questions) 2011-11-15 04:09:58 PST
Seems reasonable
Comment 10 User image :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 User image Mark Banner (:standard8) 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 User image :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 User image :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 User image Mark Banner (:standard8) 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 User image :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 User image Mark Banner (:standard8) 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 User image :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 User image :aceman 2011-12-02 12:59:25 PST
Created attachment 578688 [details] [diff] [review]
patch for nsMsgFilterService.cpp
Comment 19 User image :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 User image 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 User image :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 User image :aceman 2011-12-05 14:09:10 PST
Created attachment 579159 [details] [diff] [review]
v2, patch for nsMsgFilterService.cpp
Comment 23 User image Mark Banner (:standard8) 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 User image :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 User image 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 User image :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 User image :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.