+++ This bug was initially created as a clone of Bug #821253 comment 3 +++ (READ COMMENTS FROM BOTTOM) neil@parkwaycc.co.uk 2012-12-14 17:10:20 CET (In reply to :aceman from comment #6) > Isn't "nsTArray<nsCOMPtr<nsIMsgRuleAction> > actionList" already the > internal member as type-safe array ? So what are those 2 steps? The first step would be to add the actionsCount attribute and remove the actionsList attribute. The internal member does not have to be changed to achieve this change. However once this is done there are no external restrictions affecting the type of the internal member, so it can now be turned into a type-safe array rather than a scriptable array. Comment 6 :aceman 2012-12-14 16:58:02 CET Isn't "nsTArray<nsCOMPtr<nsIMsgRuleAction> > actionList" already the internal member as type-safe array ? So what are those 2 steps? Comment 5 neil@parkwaycc.co.uk 2012-12-14 15:36:20 CET (In reply to aceman from comment #4) > I'll create a new bug for the change of actionList to > nsTArray<nsCOMPtr<nsIMsgRuleAction> > as Neil suggests. Do I understand it > correctly, that we would then add a actionsCount attribute, but remove the > actionList attribute? Yes, that would be the first step; the second step would be to update the internal member to a type-safe array. (I say two steps because you may find it helpful to split the work into two patches.) Comment 4 :aceman 2012-12-14 09:05:05 CET I'll create a new bug for the change of actionList to nsTArray<nsCOMPtr<nsIMsgRuleAction> > as Neil suggests. Do I understand it correctly, that we would then add a actionsCount attribute, but remove the actionList attribute? Comment 3 neil@parkwaycc.co.uk 2012-12-14 01:29:57 CET Comment on attachment 692032 [details] [diff] [review] [diff] [review] patch > nsMsgFilter::nsMsgFilter(): > m_temporary(false), > m_unparseable(false), > m_filterList(nullptr), > m_expressionTree(nullptr) > { > NS_NewISupportsArray(getter_AddRefs(m_termList)); >- NS_NewISupportsArray(getter_AddRefs(m_actionList)); >+ m_actionList = do_CreateInstance(NS_ARRAY_CONTRACTID); It's possible to add this to the initialiser list i.e. nsMsgFilter::nsMsgFilter(): m_temporary(false), m_unparseable(false), m_filterList(nullptr), m_actionList(do_CreateInstance(NS_ARRAY_CONTRACTID)), m_expressionTree(nullptr) > NS_IMETHODIMP > nsMsgFilter::GetActionAt(int32_t aIndex, nsIMsgRuleAction **aAction) [Given that this exists, I wonder why we use filter.actions.queryElementAt instead of filter.getActionAt; if we go all the way and expose the length we can then switch to a type-safe internal array.]

I'll do this after bug 821253, but first I'd really like to hear comment from rkent, if this is OK. It may also affect extensions that use that attribute and he is knowledgeable about those.

I found a few extensions that use the filter actionList attribute. However, those would have to change in any case if actionList is converted into an nsIArray, right? So once changes are needed, then they can change it almost as easily to any viable access method. The only argument I could come up with that is against exposing the count only instead of the array is that if someone wanted to write an extension to manipulate the action list, they would have to read the existing array into a js array, manipulate that js array, clear the C++ array, then append new elements into the C++ array. I could not find any existing extensions that try to do that. So I don't really have an opinion either way.

(In reply to Kent James (:rkent) from comment #2) > I found a few extensions that use the filter actionList attribute. However, > those would have to change in any case if actionList is converted into an > nsIArray, right? So once changes are needed, then they can change it almost > as easily to any viable access method. Yes. > The only argument I could come up with that is against exposing the count > only instead of the array is that if someone wanted to write an extension to > manipulate the action list, they would have to read the existing array into > a js array, manipulate that js array, clear the C++ array, then append new > elements into the C++ array. > > I could not find any existing extensions that try to do that. So I don't > really have an opinion either way. Technically the number of actions is unlimited since recently. But there still probably aren't many filters using many actions (think normal filters do use up to 5). Depends on what you consider a problem here, speed would probably not be it. Only the amount of code needed to do the work. Also I think Neil considered such direct access to the array (in case of search terms in another bug) to be dangerous and should be deprecated. So when I do the change here I will see if there are any in-tree callers that do the manipulation of the array you mentioned. Then we can reconsider again.

So there were no in-tree users that needed to manipulate the action list directly.

Patch to be applied on top of bug 821253 (to be landed shortly).

Comment on attachment 695244 [details] [diff] [review] patch >+ NS_ENSURE_ARG_MAX(aIndex, m_actionList.Length() - 1); Not sure that this does the right thing for an empty list.

I don't a filter with 0 actions can exist. But even then, aIndex is unsigned, it can't be below 0. So it will fail being <= -1 in this check. Or does the NS_ENSURE_ARG_MAX macro contain some signed/unsigned integer wrapping (-1 => MAXINT -1 ?).

(In reply to aceman from comment #7) > I don't a filter with 0 actions can exist. But even then, aIndex is > unsigned, it can't be below 0. So it will fail being <= -1 in this check. > Or does the NS_ENSURE_ARG_MAX macro contain some signed/unsigned integer > wrapping (-1 => MAXINT -1 ?). No need to blame the macro; your code contains unsigned integer wrapping.

Then we have a similar problem at http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterList.cpp#927 from where I copied this.

Filed bug 824296 for comment 9.

Comment on attachment 695256 [details] [diff] [review] patch v2 Review of attachment 695256 [details] [diff] [review]: ----------------------------------------------------------------- r+=me with one small change requested. ::: mailnews/base/search/src/nsMsgFilter.cpp @@ +383,5 @@ > NS_ENSURE_ARG_POINTER(aAction); > + NS_ENSURE_ARG(aIndex < m_actionList.Length()); > + > + NS_IF_ADDREF(*aAction = m_actionList[aIndex]); > + return NS_OK; It appears to me that you have introduced a small change in behavior here. There is a rule (often broken) that requests to get pointers will return a failed rv if the pointer is null. The previous code followed that convention, while the new code returns NS_OK with an invalid pointer. Could you add before the return: NS_ENSURE_TRUE(*aAction, NS_ERROR_ILLEGAL_VALUE);

(In reply to Kent James from comment #12) > (From update of attachment 695256 [details] [diff] [review]) > > NS_ENSURE_ARG_POINTER(aAction); > > + NS_ENSURE_ARG(aIndex < m_actionList.Length()); > > + > > + NS_IF_ADDREF(*aAction = m_actionList[aIndex]); > > + return NS_OK; > > It appears to me that you have introduced a small change in behavior here. > There is a rule (often broken) that requests to get pointers will return a > failed rv if the pointer is null. The previous code followed that > convention, while the new code returns NS_OK with an invalid pointer. Could > you add before the return: > > NS_ENSURE_TRUE(*aAction, NS_ERROR_ILLEGAL_VALUE); I would reorder the lines slightly resulting in the following: NS_ENSURE_TRUE(*aAction = m_actionList[aIndex], NS_ERROR_ILLEGAL_VALUE); NS_ADDREF(*aAction); return NS_OK; (I'm not actually convinced that this will be a problem because AppendAction null-checks aAction on the way in anyway.)

Thanks.