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

RESOLVED FIXED in Thunderbird 11.0

Status

MailNews Core
Backend
RESOLVED FIXED
10 years ago
a year ago

People

(Reporter: Joey Minta, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 11.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Attachment #298516 - Flags: superreview?(dmose)
Attachment #298516 - Flags: review?(bugzilla)
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 ;-)
Attachment #298516 - Flags: review?(bugzilla) → review-
(Reporter)

Updated

9 years ago
Attachment #298516 - Flags: superreview?(dmose)
Product: Core → MailNews Core
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).
Attachment #298516 - Attachment is obsolete: true
Whiteboard: [has draft patch][patchlove]
Severity: normal → critical
serge, does this get picked up in one of your other bugs?
I am particularly interested in seeing the compact and filter items land.
(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 :-|
bienvenu, can you drive in the couple lines for nsMsgFolderCompactor.cpp?

Comment 6

6 years ago
(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.
Assignee: jminta → sgautherie.bz
Summary: null-arg checks in nsMsgFilterService.cpp, nsMsgFolderCompactor.cpp, and nsMsgPrintEngine.cpp → add null-arg checks in nsMsgFilterService.cpp, nsMsgFolderCompactor.cpp, and nsMsgPrintEngine.cpp
(Assignee)

Comment 7

6 years ago
Serge, Joey, will you continue with this patch?
Or should I pick it up?
Assignee: sgautherie.bz → acelists
(Assignee)

Comment 8

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

Comment 10

6 years ago
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.
Attachment #574643 - Flags: review?(mbanner)
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".
Attachment #574643 - Flags: review?(mbanner) → review-
(Assignee)

Comment 12

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

Comment 13

6 years ago
Created attachment 576824 [details] [diff] [review]
2nd try, patch for PrintEngine and FolderCompactor

What about this?
Attachment #574643 - Attachment is obsolete: true
Attachment #576824 - Flags: review?(mbanner)
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.
Attachment #576824 - Flags: review?(mbanner) → review+
(Assignee)

Comment 15

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

Updated

6 years ago
Severity: critical → normal
Keywords: crash
Whiteboard: [has draft patch][patchlove]
(Assignee)

Comment 17

6 years ago
Created attachment 578687 [details] [diff] [review]
final patch for PrintEngine and FolderCompactor [check this in]

Carrying over r+mbanner from comment 14.
Attachment #578687 - Flags: review+
(Assignee)

Comment 18

6 years ago
Created attachment 578688 [details] [diff] [review]
patch for nsMsgFilterService.cpp
Attachment #578688 - Flags: review?(mbanner)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [checkin patch 578687]
(Assignee)

Comment 19

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

6 years ago
in parameters are allowed to be null. Is there some reason we think rootFolder isn't allowed to be null.
(Assignee)

Comment 21

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

Comment 22

6 years ago
Created attachment 579159 [details] [diff] [review]
v2, patch for nsMsgFilterService.cpp
Attachment #578688 - Attachment is obsolete: true
Attachment #579159 - Flags: review?(mbanner)
Attachment #578688 - Flags: review?(mbanner)
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.
Attachment #579159 - Flags: review?(mbanner) → review+
(Assignee)

Comment 24

6 years ago
Created attachment 582875 [details] [diff] [review]
final patch for nsMsgFilterService.cpp [check this in too]

Carrying over r=Standard8.
Attachment #576824 - Attachment is obsolete: true
Attachment #579159 - Attachment is obsolete: true
Attachment #582875 - Flags: review+
(Assignee)

Updated

6 years ago
Whiteboard: [checkin patch 578687] → [checkin patch 578687 and 582875]
https://hg.mozilla.org/comm-central/rev/d84500876a3c
https://hg.mozilla.org/comm-central/rev/587bf3f19e32
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin patch 578687 and 582875]
Target Milestone: --- → Thunderbird 11.0

Updated

6 years ago
Depends on: 712312
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?
(Assignee)

Comment 27

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

Comment 28

6 years ago
Fixed in bug 712312, thanks to Mike.
You need to log in before you can comment on or make changes to this bug.