Last Comment Bug 180312 - nsMsgFilterList::nsMsgFilterList() should not call NS_NewISupportsArray in constructor
: nsMsgFilterList::nsMsgFilterList() should not call NS_NewISupportsArray in co...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: :aceman
:
Mentors:
Depends on: 963362
Blocks:
  Show dependency treegraph
 
Reported: 2002-11-15 07:40 PST by Pratik
Modified: 2014-01-24 02:26 PST (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (19.64 KB, patch)
2012-01-23 10:58 PST, :aceman
standard8: feedback+
Details | Diff | Review
broken patch (21.57 KB, patch)
2012-04-17 13:07 PDT, :aceman
standard8: feedback+
Details | Diff | Review
patch v3 (20.61 KB, patch)
2012-05-26 05:32 PDT, :aceman
no flags Details | Diff | Review
patch v4 (16.02 KB, patch)
2012-06-19 03:03 PDT, Mark Banner (:standard8)
standard8: review+
acelists: feedback+
Details | Diff | Review
patch v5 (21.67 KB, patch)
2012-06-20 11:52 PDT, :aceman
mozilla: superreview+
Details | Diff | Review
patch v6 (21.65 KB, patch)
2012-06-25 11:27 PDT, :aceman
acelists: review+
acelists: superreview+
Details | Diff | Review

Description Pratik 2002-11-15 07:40:11 PST
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) PTO Until July 5th 2002-12-24 05:55:19 PST
m_filters should be an nsCOMArray<nsIMsgFilter>
Comment 2 (not reading, please use seth@sspitzer.org instead) 2003-05-08 11:08:14 PDT
mass re-assign.
Comment 3 (not reading, please use seth@sspitzer.org instead) 2007-06-21 15:05:03 PDT
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) 2008-06-20 10:45:13 PDT
Filter on "Nobody_NScomTLD_20080620"
Comment 5 Wayne Mery (:wsmwk, NI for questions) 2011-03-22 15:41:22 PDT
(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 2011-12-02 13:54:25 PST
(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] 2011-12-07 03:14:41 PST
(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) 2011-12-09 04:04:36 PST
(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] 2012-01-10 07:32:31 PST
:aceman see ^^^^
Comment 10 :aceman 2012-01-10 07:41:29 PST
Thanks, looks a bit involved but I can try it.
Comment 11 :aceman 2012-01-23 10:58:06 PST
Created attachment 590789 [details] [diff] [review]
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 2012-01-23 10:59:03 PST
I don't know how to fix that one. Maybe I use the variable in some bad way.
Comment 13 Mark Banner (:standard8) 2012-02-20 13:28:58 PST
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) 2012-02-20 13:31:26 PST
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 2012-02-23 13:15:27 PST
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 2012-04-12 13:32:58 PDT
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) 2012-04-17 02:56:31 PDT
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 2012-04-17 03:13:36 PDT
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 2012-04-17 13:07:16 PDT
Created attachment 615839 [details] [diff] [review]
broken patch

OK, this is the current state.
Comment 20 Mark Banner (:standard8) 2012-05-25 02:45:29 PDT
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 2012-05-25 02:52:58 PDT
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) 2012-05-25 02:55:39 PDT
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 2012-05-25 03:18:54 PDT
Skip Dump() = remove it?
Comment 24 Mark Banner (:standard8) 2012-05-25 03:28:18 PDT
(In reply to :aceman from comment #23)
> Skip Dump() = remove it?

yes :-)
Comment 25 :aceman 2012-05-26 05:32:28 PDT
Created attachment 627471 [details] [diff] [review]
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) 2012-06-19 03:03:41 PDT
Created attachment 634327 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-06-19 03:13:39 PDT
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 2012-06-19 12:47:13 PDT
Comment on attachment 634327 [details] [diff] [review]
patch v4

Thanks, this seems to work for me!
Comment 29 Mark Banner (:standard8) 2012-06-19 13:33:19 PDT
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 2012-06-19 13:45:52 PDT
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) 2012-06-19 13:53:48 PDT
(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 2012-06-19 14:36:14 PDT
Thanks, I'll check that out.
Comment 33 David :Bienvenu 2012-06-19 15:08:07 PDT
Comment on attachment 634327 [details] [diff] [review]
patch v4

clearing review request in favor of newer patch.
Comment 34 :aceman 2012-06-20 11:52:22 PDT
Created attachment 634993 [details] [diff] [review]
patch v5

Hopefully done.
Comment 35 David :Bienvenu 2012-06-25 07:59:01 PDT
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 2012-06-25 11:27:51 PDT
Created attachment 636405 [details] [diff] [review]
patch v6

Thanks!
Carrying over r=standard8, sr=bienvenu.
Comment 37 Ryan VanderMeulen [:RyanVM] 2012-06-26 18:04:52 PDT
https://hg.mozilla.org/comm-central/rev/9631a5ce5064

Note You need to log in before you can comment on or make changes to this bug.