Last Comment Bug 821743 - Remove nsISupportsArray actionList attribute in nsIMsgFilter
: Remove nsISupportsArray actionList attribute in nsIMsgFilter
Status: RESOLVED FIXED
: addon-compat
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/i...
Depends on: 821253
Blocks: 394167 436089 747284 824296
  Show dependency treegraph
 
Reported: 2012-12-14 08:51 PST by :aceman
Modified: 2012-12-30 16:36 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.07 KB, patch)
2012-12-22 11:05 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (13.06 KB, patch)
2012-12-22 13:36 PST, :aceman
neil: review+
rkent: review+
Details | Diff | Splinter Review
patch v3 (13.13 KB, patch)
2012-12-27 13:46 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-12-14 08:51:58 PST
+++ 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.]
Comment 1 :aceman 2012-12-14 08:53:44 PST
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.
Comment 2 Kent James (:rkent) 2012-12-18 14:19:29 PST
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.
Comment 3 :aceman 2012-12-19 00:19:36 PST
(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.
Comment 4 :aceman 2012-12-22 11:05:32 PST
Created attachment 695244 [details] [diff] [review]
patch

So there were no in-tree users that needed to manipulate the action list directly.
Comment 5 :aceman 2012-12-22 11:07:12 PST
Patch to be applied on top of bug 821253 (to be landed shortly).
Comment 6 neil@parkwaycc.co.uk 2012-12-22 12:51:03 PST
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.
Comment 7 :aceman 2012-12-22 13:05:28 PST
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 ?).
Comment 8 neil@parkwaycc.co.uk 2012-12-22 13:09:52 PST
(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.
Comment 9 :aceman 2012-12-22 13:35:50 PST
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.
Comment 10 :aceman 2012-12-22 13:36:45 PST
Created attachment 695256 [details] [diff] [review]
patch v2
Comment 11 :aceman 2012-12-23 05:59:16 PST
Filed bug 824296 for comment 9.
Comment 12 Kent James (:rkent) 2012-12-27 10:09:59 PST
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);
Comment 13 neil@parkwaycc.co.uk 2012-12-27 10:36:52 PST
(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.)
Comment 14 :aceman 2012-12-27 13:46:50 PST
Created attachment 696127 [details] [diff] [review]
patch v3

Thanks.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-12-30 16:36:47 PST
https://hg.mozilla.org/comm-central/rev/abb6e83e69a8

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