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

RESOLVED FIXED in Thunderbird 11.0

Status

MailNews Core
Backend
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 11.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

It it useful?
(Assignee)

Comment 1

6 years ago
Created attachment 578726 [details] [diff] [review]
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 :)
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #578726 - Flags: review?(mbanner)
(Assignee)

Comment 2

6 years ago
Created attachment 579401 [details] [diff] [review]
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.
Attachment #578726 - Attachment is obsolete: true
Attachment #578726 - Flags: review?(mbanner)
Attachment #579401 - Flags: review?(mbanner)
(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.
(Assignee)

Comment 4

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

Comment 7

6 years ago
Created attachment 582305 [details] [diff] [review]
v3 of patch

Carrying over r=standard8.
Attachment #579401 - Attachment is obsolete: true
Attachment #582305 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/5cca813898cc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 9

6 years ago
Sid0, thanks for merging, but you forgot to set the milestone. Is it TB11 as other patches you landed today?
Yep, sorry.
Target Milestone: --- → Thunderbird 11.0

Comment 11

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

Updated

6 years ago
Blocks: 713429
(Assignee)

Comment 12

6 years ago
I am sorry about that. I must memorize those macros take negated expressions when converting existing code...
Depends on: 735940
Depends on: 742155
No longer depends on: 742155
(Assignee)

Updated

5 years ago
Blocks: 755658
You need to log in before you can comment on or make changes to this bug.