180312 - nsMsgFilterList::nsMsgFilterList() should not call NS_NewISupportsArray in constructor

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Pratik]

timeless asked me to file this bug. Basically nsMsgFilterList constructor calls
NS_NewISupportsArray and that can fail and cause problems. As part of a patch
for bug 179775, I'm adding an assertion after the call. timless says we should
be using an init() method that handles the failure.

fyi, the problem is also present in nsMsgFilter class.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

m_filters should be an nsCOMArray<nsIMsgFilter>

Comment 2

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 3

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 4

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to comment #0)
> timeless asked me to file this bug. Basically nsMsgFilterList constructor calls
> NS_NewISupportsArray and that can fail and cause problems. As part of a patch
> for bug 179775, I'm adding an assertion after the call. timless says we should
> be using an init() method that handles the failure.
> 
> fyi, the problem is also present in nsMsgFilter class.

(In reply to comment #1)
> m_filters should be an nsCOMArray<nsIMsgFilter>

still valid - http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterList.cpp#75

Comment 6

[:aceman]

(In reply to Jonas Sicking (:sicking) from comment #1)
> m_filters should be an nsCOMArray<nsIMsgFilter>

Any idea how to tackle this?

Comment 7

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #6)
> (In reply to Jonas Sicking (:sicking) from comment #1)
> > m_filters should be an nsCOMArray<nsIMsgFilter>
> 
> Any idea how to tackle this?

James ?

Comment 8

[Mark Banner (:standard8)]

(In reply to :aceman from comment #6)
> (In reply to Jonas Sicking (:sicking) from comment #1)
> > m_filters should be an nsCOMArray<nsIMsgFilter>
> 
> Any idea how to tackle this?

So slight change. Generally these days we prefer to use:

nsTArray<nsCOMPtr<nsIMsgFilter> >

See:

http://mxr.mozilla.org/comm-central/search?string=nsTArray.*nsCOMPtr&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

I would tackle this by first changing the member variable in nsMsgFilterList.h from

nsCOMPtr<nsISupportsArray> m_filters;

to


nsTArray<nsCOMPtr<nsIMsgFilter> > m_filters;

You can then get away from the extra dynamic allocation in the constructor.

Then you've got two options: 1) work out which functions to replace in the file manually, or 2) compile, see where it breaks, fix it and repeat. Once you get to know the pattern, it generally isn't too bad.

A few hints:

- pointers change to member functions, i.e. '->' changes to '.'
- ->Count(&result) gets replaced by result = m_filters.Length()
- ElementAt(...) stays the same, or could be replaced by m_filters[...]

Comment 9

[Ludovic Hirlimann [:Usul]]

:aceman see ^^^^

Comment 10

[:aceman]

Thanks, looks a bit involved but I can try it.

Comment 11

[:aceman]

Attachment: patch

I have converted the variable according to comment 8. It almost compiles, I get this last error:

In file included from
../../../../mozilla/dist/include/nsReadableUtils.h:51:0,
                 from ../../../../mozilla/dist/include/nsString.h:53,
                 from ../../../../mozilla/dist/include/nsStringGlue.h:49,
                 from ../../../../mozilla/dist/include/nsTextFormatter.h:70,
                 from
mailnews/base/search/src/nsMsgFilterList.cpp:42:
../../../../mozilla/dist/include/nsTArray.h: In static member function
'static void nsTArrayElementTraits<E>::Construct(E*, const A&) [with A =
nsIMsgFilter, E = nsCOMPtr<nsIMsgFilter>]':
../../../../mozilla/dist/include/nsTArray.h:1228:7:   instantiated from
'void nsTArray<E, Alloc>::AssignRange(nsTArray<E, Alloc>::index_type,
nsTArray<E, Alloc>::size_type, const Item*) [with Item = nsIMsgFilter, E
= nsCOMPtr<nsIMsgFilter>, Alloc = nsTArrayDefaultAllocator, nsTArray<E,
Alloc>::index_type = unsigned int, nsTArray<E, Alloc>::size_type =
unsigned int]'
../../../../mozilla/dist/include/nsTArray.h:738:5:   instantiated from
'elem_type* nsTArray<E, Alloc>::ReplaceElementsAt(nsTArray<E,
Alloc>::index_type, nsTArray<E, Alloc>::size_type, const Item*,
nsTArray<E, Alloc>::size_type) [with Item = nsIMsgFilter, E =
nsCOMPtr<nsIMsgFilter>, Alloc = nsTArrayDefaultAllocator, elem_type =
nsCOMPtr<nsIMsgFilter>, nsTArray<E, Alloc>::index_type = unsigned int,
nsTArray<E, Alloc>::size_type = unsigned int]'
../../../../mozilla/dist/include/nsTArray.h:759:47:   instantiated from
'elem_type* nsTArray<E, Alloc>::ReplaceElementAt(nsTArray<E,
Alloc>::index_type, const Item&) [with Item = nsIMsgFilter*, E =
nsCOMPtr<nsIMsgFilter>, Alloc = nsTArrayDefaultAllocator, elem_type =
nsCOMPtr<nsIMsgFilter>, nsTArray<E, Alloc>::index_type = unsigned int]'
mailnews/base/search/src/nsMsgFilterList.cpp:1017:49:
 instantiated from here
../../../../mozilla/dist/include/nsTArray.h:357:5: error: no matching
function for call to 'nsCOMPtr<nsIMsgFilter>::nsCOMPtr(const nsIMsgFilter&)'
../../../../mozilla/dist/include/nsCOMPtr.h:626:7: note: candidates are:
nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr_helper&) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:618:7: note:
 nsCOMPtr<T>::nsCOMPtr(const nsGetServiceByContractIDWithError&) [with T
= nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:610:7: note:
 nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:602:7: note:
 nsCOMPtr<T>::nsCOMPtr(const nsGetServiceByCIDWithError&) [with T =
nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:594:7: note:
 nsCOMPtr<T>::nsCOMPtr(nsGetServiceByCID) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:586:7: note:
 nsCOMPtr<T>::nsCOMPtr(const nsQueryInterfaceWithError&) [with T =
nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:578:7: note:
 nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:570:7: note:
 nsCOMPtr<T>::nsCOMPtr(const already_AddRefed<T>&) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:560:7: note:
 nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:551:7: note:
 nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr<T>&) [with T = nsIMsgFilter]
../../../../mozilla/dist/include/nsCOMPtr.h:544:7: note:
 nsCOMPtr<T>::nsCOMPtr() [with T = nsIMsgFilter]
gmake[6]: *** [nsMsgFilterList.o] Error 1

Comment 12

[:aceman]

I don't know how to fix that one. Maybe I use the variable in some bad way.

Comment 13

[Mark Banner (:standard8)]

The error is in SetFilterAt, it is because of this line:

  m_filters.ReplaceElementAt(filterIndex, filter);

The "filter" is an nsIMsgFilter*, whereas to replace the element it wants to use nsCOMPtr<nsIMsgFilter>.

To fix this, I believe its safe to do:

  m_filters[filterIndex] = filter;

m_filters[filterIndex] will resolve to nsCOMPtr<nsIMsgFilter> and then the equals will apply to that nsCOMPtr. Hence assigning the filter to that nsCOMPtr.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 590789 [details] [diff] [review]
patch

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

I've not looked at this in detail, but you know I agree with the idea.

::: mailnews/base/search/src/nsMsgFilter.cpp
@@ +652,1 @@
>  nsMsgFilter::SetFilterList(nsIMsgFilterList *filterList)

note: you'll need to return a value from this function because of the return value change.

Comment 15

[:aceman]

Thanks, 'm_filters[filterIndex] = filter' worked.
It now compiles all, except the Dump function (either needs to be converted to nsIMsgFolder or fix the getMsgFolder function where I need to convert between nsIMsgFolder and nsMsgFolder, which I couldn't do for now).

Nevertheless, many filter tests fail with this so there must be something busted in the patch.

Comment 16

[:aceman]

Standard8, there is something wrong with the assignment in SetFilterAt (comment 13). Everything compiles, but e.g. calling MoveFilterAt(), which calls SetFilterAt(), TB crashes at the assignment (at least gdb says so).
So either the assignment is wrong, or the manual swapping of elements in MoveFilterAt() is wrong.

Comment 17

[Mark Banner (:standard8)]

Can you attach an updated patch which I can take a look at?

I think it'd be good if you could also try writing some xpcshell tests for the functions you're changing on nsIMsgFilterList. This should be reasonably simple to do. We've already got some tests that include filters:

http://mxr.mozilla.org/comm-central/search?string=msgfilter&find=/mailnews/.*test_

You wouldn't need to do test what the filter actually does, just that the filters get moved around and set appropriately.

Comment 18

[:aceman]

The existing tests cover the code nicely so they all fail :)
I was able to see the crash just playing with the UI and pressing the Move down (up) buttons in filter editor. I'll attach the patch.

Comment 19

[:aceman]

Attachment: broken patch

OK, this is the current state.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 615839 [details] [diff] [review]
broken patch

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

Sorry for the delay in getting to this.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +982,2 @@
>  
> +  *filter = m_filters[filterIndex];

This should be NS_IF_ADDREF(*filter = m_filters[filterIndex]);

The issue is that we're just getting the value from the nsCOMPtr stored in m_filters, and assigning to *filter doesn't actually addref it, so we must do that manually. Otherwise the callers assume it has already been addrefed, and when they release the local nsCOMPtr<> reference, they would decrement the overall reference count, and it'd hit zero too early, and the data being released at the wrong time.

With that fixed the unit tests pass for me.

Comment 21

[:aceman]

Oh, thanks.
Is there anything else I should change in the patch?
What about the Dump function? I was not able to convert it so it is commented out now.

Comment 22

[Mark Banner (:standard8)]

I think we can skip the dump function. I don't think there's anything else to change, just drop the commented out sections, of course.

Comment 23

[:aceman]

Skip Dump() = remove it?

Comment 24

[Mark Banner (:standard8)]

(In reply to :aceman from comment #23)
> Skip Dump() = remove it?

yes :-)

Comment 25

[:aceman]

Attachment: patch v3

Thanks, so it seems to work better now.

But it still is not right. TB crashes when opening the filter list and choosing a IMAP server (on the second attempt) and also at closing the filter list after editing a filter.

xpcshell tests pass for me, except some of test_viewWrapper_*.js.

Comment 26

[Mark Banner (:standard8)]

Attachment: patch v4

Ok, got it. SaveTextFilters had an NS_RELEASE that was no longer necessary now the pointer in that function had been switched to being an nsCOMPtr - nsCOMPtr automatically decrements the reference count, so NS_RELEASE would have been doing an additional decrement which could obviously cause values being released too early.

This version also fixes a bit of bitrot.

I've tried your crash cases and I now can't reproduce them. Can you check as well, and if so I think this is ready for r+.

Comment 27

[[e-mail]]

Comment on attachment 634327 [details] [diff] [review]
patch v4

We used to have a compile-time check that would stop you from calling NS_RELEASE on a smart pointer :-(

Comment 28

[:aceman]

Comment on attachment 634327 [details] [diff] [review]
patch v4

Thanks, this seems to work for me!

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 634327 [details] [diff] [review]
patch v4

I went through this earlier, so I'm glad its not crashing now and I can just give r+.

Asking David for sr for the minor additions to the interface.

Comment 30

[David :Bienvenu]

Comment on attachment 634327 [details] [diff] [review]
patch v4

why not just remove the readonly aspect of the filterList attribute instead of adding a SetFilterList method?

Comment 31

[Mark Banner (:standard8)]

(In reply to David :Bienvenu from comment #30)
> Comment on attachment 634327 [details] [diff] [review]
> patch v4
> 
> why not just remove the readonly aspect of the filterList attribute instead
> of adding a SetFilterList method?

Well spotted David, yes we should do that.

Comment 32

[:aceman]

Thanks, I'll check that out.

Comment 33

[David :Bienvenu]

Comment on attachment 634327 [details] [diff] [review]
patch v4

clearing review request in favor of newer patch.

Comment 34

[:aceman]

Attachment: patch v5

Hopefully done.

Comment 35

[David :Bienvenu]

Comment on attachment 634993 [details] [diff] [review]
patch v5

looks reasonable, a few nits:

better style would be

if (NS_SUCCCEEDED(GetFilterAt...) && filter);

+    if (GetFilterAt(i, getter_AddRefs(filter)) == NS_OK && filter != nsnull)

remove commented out line:

-  nsMsgFilter *filter = static_cast<nsMsgFilter *>(aFilter);
+//  nsMsgFilter *filter = static_cast<nsMsgFilter *>(aFilter);

spaces around =, and after second ;

+    for (PRUint32 i=0; i< numSearchTerms;i++)

Comment 36

[:aceman]

Attachment: patch v6

Thanks!
Carrying over r=standard8, sr=bienvenu.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9631a5ce5064