The default bug view has changed. See this FAQ.

Remove nsISupportsArray actionList attribute in nsIMsgFilter

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Filters
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({addon-compat})

Trunk
Thunderbird 20.0
addon-compat
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

13.13 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
+++ 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.]
(Assignee)

Comment 1

4 years ago
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.
Flags: needinfo?(kent)

Comment 2

4 years ago
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.
Flags: needinfo?(kent)
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 695244 [details] [diff] [review]
patch

So there were no in-tree users that needed to manipulate the action list directly.
Attachment #695244 - Flags: review?(neil)
(Assignee)

Updated

4 years ago
Attachment #695244 - Flags: review?(kent)
(Assignee)

Comment 5

4 years ago
Patch to be applied on top of bug 821253 (to be landed shortly).
Status: NEW → ASSIGNED
Keywords: addon-compat
(Assignee)

Updated

4 years ago
Blocks: 747284

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
Created attachment 695256 [details] [diff] [review]
patch v2
Attachment #695244 - Attachment is obsolete: true
Attachment #695244 - Flags: review?(neil)
Attachment #695244 - Flags: review?(kent)
Attachment #695256 - Flags: review?(neil)

Updated

4 years ago
Attachment #695256 - Flags: review?(neil) → review+
(Assignee)

Comment 11

4 years ago
Filed bug 824296 for comment 9.
Blocks: 824296
(Assignee)

Updated

4 years ago
Attachment #695256 - Flags: review?(kent)

Comment 12

4 years ago
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);
Attachment #695256 - Flags: review?(kent) → review+
(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.)
(Assignee)

Comment 14

4 years ago
Created attachment 696127 [details] [diff] [review]
patch v3

Thanks.
Attachment #695256 - Attachment is obsolete: true
Attachment #696127 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/abb6e83e69a8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.