413521 - add null-arg checks in nsMsgFilterService.cpp, nsMsgFolderCompactor.cpp, and nsMsgPrintEngine.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: 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)]

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] (NOT official MoCo now)]

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)]

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)]

(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)]

bienvenu, can you drive in the couple lines for nsMsgFolderCompactor.cpp?

Comment 6

[David :Bienvenu]

(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]

Serge, Joey, will you continue with this patch?
Or should I pick it up?

Comment 8

[:aceman]

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)]

Seems reasonable

Comment 10

[:aceman]

Attachment: 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)]

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]

(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]

Attachment: 2nd try, patch for PrintEngine and FolderCompactor

What about this?

Comment 14

[Mark Banner (:standard8)]

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]

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)]

(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]

Attachment: final patch for PrintEngine and FolderCompactor [check this in]

Carrying over r+mbanner from comment 14.

Comment 18

[:aceman]

Attachment: patch for nsMsgFilterService.cpp

Comment 19

[:aceman]

--- 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]

in parameters are allowed to be null. Is there some reason we think rootFolder isn't allowed to be null.

Comment 21

[:aceman]

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]

Attachment: v2, patch for nsMsgFilterService.cpp

Comment 23

[Mark Banner (:standard8)]

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]

Attachment: final patch for nsMsgFilterService.cpp [check this in too]

Carrying over r=Standard8.

Comment 25

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/comm-central/rev/d84500876a3c
https://hg.mozilla.org/comm-central/rev/587bf3f19e32

Comment 26

[Mike Conley (:mconley) (:⚙️)]

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]

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]

Fixed in bug 712312, thanks to Mike.