707306 - add null-arg checks in nsMsgFilter.cpp, nsMsgFilterList.cpp, nsMsgSearchAdapter.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[:aceman]

In the attached patch I added some checks to NS_IMETHODIMP functions that lacked checks for null arguments passed to them.

It it useful?

Comment 1

[:aceman]

Attachment: suggested patch

I made this patch in the same style as other bugs linked from bug 412109.
I do not have any proof these changes are needed (any read crash). I added it as a robustness measure. I think I read in other bugs that interface methods should check the arguments. Please check if this has any value.
It seems this caused about 3 test failures. I will look into them only if the patch is deemed useful. Otherwise I can just scrap it :)

Comment 2

[:aceman]

Attachment: v2, suggested patch

Sorry, the checks in the original patch were too strict, caused some test failures. This patch add less checks, only on safer spots.

The change of code in nsMsgFilterList.cpp, starting at line 1127 adds braces to fix a compiler warning (possible ambiguous else) and reorders code to fold duplicated code. I hope I didn't change the logic of it. There is a logic change that now if rv fails and the function returns, *found is now true (instead of false previously). But the callers seem to check if rv failed so should ignore the returned *found.

Comment 3

[Mark Banner (:standard8)]

(In reply to :aceman from comment #0)
> In the attached patch I added some checks to NS_IMETHODIMP functions that
> lacked checks for null arguments passed to them.
> 
> It it useful?

Generally adding the checks means we'll obey xpcom rules a bit more, and it'll make it better for extension development, and stop ourselves doing the wrong thing. However, I don't see doing these on a mass basis (unless we're using automation) highly beneficial over fixing other bugs and updating code using obsolete values.

Comment 4

[:aceman]

OK. But I can't mostly do any other bugfixing (see other bugs assigned to me for what I can do). So if this is work I am able to do manually and have already done for these files, will you scrape it? Should I stop doing this even though I will then not do anything else? There even are fixes for obsolete values (PR_FALSE) in the patch.

Comment 5

[Mark Banner (:standard8)]

(In reply to :aceman from comment #4)
> OK. But I can't mostly do any other bugfixing (see other bugs assigned to me
> for what I can do). So if this is work I am able to do manually and have
> already done for these files, will you scrape it? Should I stop doing this
> even though I will then not do anything else? There even are fixes for
> obsolete values (PR_FALSE) in the patch.

I certainly don't want to stop you doing things for Thunderbird, I'll send you an email direct and we can think about what's best to get you doing.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 579401 [details] [diff] [review]
v2, suggested patch

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

r=Standard8 with the nits below fixed.

::: mailnews/base/search/src/nsMsgFilter.cpp
@@ +440,1 @@
>                                      (void **)getter_AddRefs(term));

nit: please move the second line of the statement so that the (void ** aligns with the t of termIndex.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +1145,5 @@
> +              *found = true;
> +          }
> +          if (*found)
> +          {
> +            if (!newFolderUri.IsEmpty())  //if we just want to match the uri's, newFolderUri will be null

Please put the comment on the previous line, and wrap it if necessary.

Comment 7

[:aceman]

Attachment: v3 of patch

Carrying over r=standard8.

Comment 8

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/comm-central/rev/5cca813898cc

Comment 9

[:aceman]

Sid0, thanks for merging, but you forgot to set the milestone. Is it TB11 as other patches you landed today?

Comment 10

[Rain [:rain1] (inactive)]

Yep, sorry.

Comment 11

[David :Bienvenu]

this broke saving of message filters, so people are losing their filters.

  NS_ENSURE_ARG(! (filterCount >= filterIndex));

should be NS_ENSURE_TRUE, or remove the ! - I'm going to land a fix for this, because of the data loss.

Comment 12

[:aceman]

I am sorry about that. I must memorize those macros take negated expressions when converting existing code...