Closed Bug 707306 Opened 13 years ago Closed 13 years ago

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

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 11.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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

It it useful?
Attached patch suggested patch (obsolete) β€” β€” Splinter Review
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)
Attached patch v2, suggested patch (obsolete) β€” β€” Splinter Review
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.
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+
Attached patch v3 of patch β€” β€” Splinter Review
Carrying over r=standard8.
Attachment #579401 - Attachment is obsolete: true
Attachment #582305 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/5cca813898cc
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
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.
Blocks: 713429
I am sorry about that. I must memorize those macros take negated expressions when converting existing code...
Depends on: 735940
No longer depends on: 742155
Blocks: 755658
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: