Closed Bug 162115 Opened 22 years ago Closed 22 years ago

nsIArray needed

Categories

(Core :: XPCOM, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.2beta

People

(Reporter: chak, Assigned: alecf)

References

Details

Attachments

(3 files, 6 obsolete files)

dougt's comments regarding the usage of this interface from a mail thread:

"This interface should *NOT* be frozen.  There is no reason why we should
support the use of this interface. The alternative would be to have the client
to use templates or come up with a new clean interface."


We need to figure out why/where current majoer embeddors are using this
interfaces and determine a course of action.
One thought - make a new nsIArray that we can freeze, and start migrating
individual consumers/interfaces over to that.
Summary: Steer embeddors away from using nsISupportsArray → Steer embeddors away from using nsISupportsArray
I like that idea.  Are you up for something like this? :-)
Chak, what is the exact requirement of embedders? How much functionality does
this array interface need to have?
more importantly, what interfaces are they using that depend on it? My guess is
that 9 times out of 10, nsISimpleEnumerator will suffice.
here's a rough strawman - I grovelled through nsISupportsArray - there are a lot
of redundant methods there!

interface nsIArray : nsISupports {
  readonly attribute unsigned long count;

  // we always want to query - we can optimize for nsISupports as well
  void elementAt(in unsigned long index, in nsIIDRef uuid,
                 [iid_is(uuid),retval] out nsQIResult result);

  unsigned long indexOf(in nsISupports element);
  unsigned long indexOfStartingAt(unsigned long index, in nsISupports element);

  nsISimpleEnumerator enumerate();
};

interface nsIMutableArray : nsIArray {
  void appendElement(in nsISupports element);
  void removeElement(in nsISupports element);

  // use index-based if possible, they're generally faster
  void insertElementAt(in unsigned long beforeIndex, in nsISupports element);
  void removeElementAt(in unsigned long index, in nsISupports element);
  void setElementAt(in unsigned long index, in nsISupports element);

  // or clear()? Resets the array
  void reset();
};

Maybe my questions are: why do embedders need an array interface?  How are they
currently using nsISupportsArray?  Would alec's interface work for them?  

nsICollection has:
PRUint32 Count ()
void Clear ()
(so i'd vote for Clear)

I can see benefits to count as an attribute, if you create the proposed classes,
could you also make nsISupportsArray consistent?
No. The whole reason for the new classes is that nsISupportsArray is all screwed
up and we're better off just leaving it as is and slowly migrating people over
where appropriate.

I blame me for "PRUint32 Count();" (I was the one who IDLized nsISupportsArray -
doh!) but what can ya do.
Blocks: 157137
In response to Doug's commnets at
http://bugzilla.mozilla.org/show_bug.cgi?id=162115#c6

nsISupportsArray is currently used by some embeddors who are overriding the PSM
UI (which is XUL based) to provide their own native UI. Some of the PSM
interface methods return nsISupportsArray and hence the embeddor ends up having
to use them. Here's a couple of usage scenarios:

Usage Scenario 1: Used in the case to fill the Cert details UI tab 
----------------

nsCOMPtr<nsISupportsArray> asn1Objects;

nsCOMPtr<nsIASN1Sequence> sequence(do_QueryInterface(object));
if (sequence) 
{
	sequence->GetASN1Objects(getter_AddRefs(asn1Objects));
	......
}

Usage Scenario 2: Used in the case to display the Cert chain
----------------
nsCOMPtr<nsIX509Cert> cert;
nsCOMPtr<nsISupportsArray> array;
cert->GetChain(getter_AddRefs(array));

the PSM interfaces could not be frozen if they are using the nsISupportsArray. 
If they want to support arrays of out parameters, then we need a simple array
class.  NOT nsISupportArray.  Thanks for the info.
ok, that one is simple, we just change the PSM interfaces to use
nsISimpleEnumerator instead. can you file a new bug about that? I'm sure kai
will help us out.
oh! just saw dougt's comment.. yeah, using simple IDL arrays will work too.. the
enumerator is probably less work for the PSM folks.
wontfix.  psm interface problem.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → WONTFIX
Reopening. I discussed with Alec.

In PSM the following happens:
- A creates an array
- B obtains the array
- some Bs keep the list and iterate multiple times
- other Bs keep the list and even need random access

The random access is needed, because the array is a data storage for elements
shown in the UI, and the stored objects are required while the user works with
the elements. See nsCertTree.cpp, mCertArray for an example.

If we are no longer allowed to use nsISupportsArray, but only
nsISimpleEnumerator, we'd end up requiring to do:
- A creates array
- B obtains enumerator
- B iterates and creates a new array
- B continues to use the new array

I would be nice to have a way to avoid the copying.

I see that consumers of nsISupportsArrays only need very basic access.
All we do is:
- Count
- ElementAt
- RemoveElementAt


I think it would be simple to implement a frozen interface as Alec suggested in
comment 5.

Option 1:
- derive nsISupportsArray from the new array class
- use new method names to avoid conflicts with those already used in nsISupportArray
- simply map the new calls directly to other calls

Option 2:
- create a small wrapper implementation for the new array class
- for now, have it store a pointer to nsISupportsArray
- simply forward the calls

While Option 1 requires us to choose new method names, it avoids the additional
(small) allocation with Option 2.


If you don't want to implement that new interface globally for Mozilla, I'd be
willing to introduce a PSM local array class to do just that.


What do you think?
Blocks: 165574
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
nsISimpleArray 
Summary: Steer embeddors away from using nsISupportsArray → nsISimpleArray needed
do we need to add the word Simple? it just seems redundant. I know we have
nsISimpleEnumerator but that is because there was already an nsIEnumerator.
Also, "Simple" implies that there there is also a more complicated array available.

How about nsIArray?

it doesn't matter to me too much.  
over to alec.
Assignee: dougt → alecf
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
Ok, I've come up with a really cool way to have this array, thanks to a comment
from sicking about how people should be using nsSupportsArray (note the lack of
"I" in that class name)

So here's what I've got

nsVoidArray             nsIArray
   \                      /
  nsInterfaceArray  nsIMutableArray
        \                /
             nsArray

The two important things here are:
nsInterfaceArray: a non-XPCOM array of XPCOM objects - all its accessors are
smart about refcounting the objects as they go in and out of the array, and the
accessors to individual objects do not refcount. In addition, this class is not
an XPCOM object, so we don't have to go through virtual methods to call into it,
and it can exist on the stack or as a class member variable without being
seperately instantiated. Since it derives from nsVoidArray, it benefits from all
the neato stuff that IT does.

nsArray: a concrete implementation of nsIMutableArray/nsIArray - this is
basically just an XPCOM wrapper around nsInterfaceArray (and actually drives
from it) so most methods are just call-throughs to nsInterfaceArray, which
mostly just calls through to nsVoidArray, making all the wrappers very thin (and
any good compiler will optimize away the overhead!)

Summary: nsISimpleArray needed → nsIArray needed
oh, on a related note, I'm trying to decide what to call the non-XPCOM array -
nsInterfaceArray is what I'm calling it now, but I'm not a huge fan. Does
nsObjectArray seem to generic?
cc'ing dougt for any suggestions (dougt, see my previous comment as well)

maybe the picture is misleading....

How can nsVoidArray derive from nsInterfaceArray when nsVoidArray doesn't want
refcounting.  maybe I just don't see the difference between a nsVoidArray and
nsArray...  (sorry for being dense).


I don't mind the name that much.  

Also, the interfaces should be as simple as possible.  If we are going to have
Gecko API which expect one of these interfaces as a parameter, then
embedders/components developers are going to have to implement these interfaces.
 (if this becomes an issue we can move this ds into xpcom/glue).
dougt: the inheritance goes the other way - nsObjectArray inherits from
nsVoidArray, and nsArray inherits from BOTH nsIArray and nsObjectArray. 

Most of the work is in nsObjectArray, and nsArray becomes more or less a
refcounted wrapper around nsObjectArray, which is more or less a syntax wrapper
around nsVoidArray.

(I decided "Interface" was a mouthful and annoying to type, plus is strange
because techincally an interface doesn't take up memory, an Object does) 

When the tree opens, I'll check in my current WIP and put links in the bug
(rather than pasting a quickly-rotting version in this bug)

The interfaces are very simple. The read-only nsIArray has only 4 methods, and
the writable nsIMutableArray adds 4 more.
ok, very cool stuff here. Talked with jag a bit, and it looks like what I'm
going to do now is nsCOMArray<> - like nsCOMPtr, you'll be able to say:

nsCOMArray<nsIContent> mChildren;

or something like that. Since it all boils down to a few calls to nsVoidArray,
the code overhead of the template should be minimal or even non-existant, and it
will be fast, because there are no virtual methods.

Then, nsIArray will be a wrapper around nsCOMArray<nsISupports>

and here's the makefile and factory changes required.
note that I have not yet implemented nsArray::Enumerate(), and I still want to
add  a method which will make an nsArray that wraps an existing nsCOMArray.. but
those aren't required just yet.
I'm just curious, I can see:

nsCOMArray.h:
PRBool InsertObjectAt(T* aObject, PRInt32 aIndex) 
PRBool ReplaceObjectAt(T* aObject, PRInt32 aIndex) 
PRBool AppendObject(T *aObject) 
PRBool RemoveObject(T *aObject) 
PRBool RemoveObjectAt(PRInt32 aIndex) 

"Object" seems redundant to me (since it's obvious we deal with "objects" here
?) why not have:

PRBool InsertAt(T* aObject, PRInt32 aIndex) 
PRBool ReplaceAt(T* aObject, PRInt32 aIndex) 
PRBool Append(T *aObject) 
PRBool Remove(T *aObject) 
PRBool RemoveAt(PRInt32 aIndex)

the same holds for nsIArray and nsIMutableArray
(for instance removeElementAt -> removeAt)
> PRBool nsCOMArray_base::ReplaceObjectAt(nsISupports* aObject, PRInt32 aIndex) {
>   nsISupports *oldObject = ObjectAt(aIndex);
>   NS_IF_RELEASE(oldObject);
>   PRBool result = mArray.ReplaceElementAt(aObject, aIndex);
>   if (result)
>     NS_ADDREF(aObject);
>   return result;
> }

Looking at http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsVoidArray.cpp, it
seems that nsVoidArray::ReplaceElementAt can fail though the previous ObjectAt
suceeded - in such a case, we would hold a possibly dead (because released once
too often) pointer at position aIndex, wouldn't we?
The handling of NULL pointers does not seem to be consistent throughout
nsCOMArray_base: For instance, RemoveObjectAt rejects to remove them from the
array, while the Insert/Append methods allow to insert them (but will probably
crash due to the NS_ADDREF instead of NS_IF_ADDREF) ...
alecf: please change all of the new files to MPL/LGPL/GPL instead of NPL/...,
new files are not supposed to be NPL.
thanks for all the great feedback folks, I'll try to tackle the comments today
and  post here when there are updates.
ok, I was going to disallow nulls in the array, but I discovered that
nsVoidArray has this habit of growing the array if you call ReplaceElementAt()
on an index beyond the current size of the array. Since this allows there to be
null elements in the array anyway, I decided to be consistent and allow nulls in
both nsArray and nsCOMArray..

I also cleaned up some other stuff, like the fact that nsArray::IndexOf() wasn't
AddRef'ing its result.. I'm going back to fix up documentation and spelling now...
ok, new changes have landed - new licenses, etc
I also added an NS_NewArray() which creates either an empty nsIArray, or an
nsIArray which makes a copy of an existing nsCOMArray<T> for use in getters

(as in

nsCOMArray<nsIContent> mImportantNodes;

nsFoo::GetImportantNodes(nsIArray** aResult)
{
  return NS_NewArray(mImportantNodes, aResult);
}

here's an updated build patch. I'm now exporting nsArray.h so that people can
get at NS_NewArray()
Attachment #100917 - Attachment is obsolete: true
you could save heap-size for nsCOMArray by using nsSmallVoidArray instead of
nsVoidArray. This class is currently somewhat wastefull when holding more then
one element, but that should be fixed anyway (filed as bug 171863)
One more small thing :). The comment in nsArray.h disallows the instantiation of
nsArray on the stack. Don't know if there any conventions regarding this, but
wouldn't it be better to make the dtor protected? This would prevent
stack-instantiation (as well as explicit deletion of an nsArray*) at
compile-time ....
ok, I finally wrote some enumerators, this patch adds them to the build. 

I wrote one for nsCOMArray<T> and one for nsIArray. see:

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArrayEnumerator.h
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArrayEnumerator.cpp

and I've just updated the source (see comment 24) to support this new
enumerator.
Attachment #101205 - Attachment is obsolete: true
nsCOMArrayEnumerator methods should not null-test mValueArray, as that member is
an array, so always non-null.

nsCOMArrayEnumerator's dtor uses NS_RELEASE, but the equivalent "ctor" (operator
new for the class) uses NS_IF_ADDREF.  The dtor should use NS_IF_RELEASE.

I think it's worth being consistent in calling NS_ADDREF(*aResult) after storing
in *aResult, instead of sometimes doing NS_ADDREF(enumer); *aResult = enumer.

Can we get all the files attached for easier reviewing?  Thanks,

/be
Attached patch all files in one "patch" (obsolete) — Splinter Review
ok, this is a listing of all the files that I've added, to make it easier to
review in one patch.

I've addressed brendan's comments (I blame the inconsistent NS_ADDREF(enumer)
on my cutting and pasting from nsEnumeratorUtils.cpp)
minor update to the patch - forgot mac changes for nsArrayEnumerator.cpp/.h,
and also make nsEnumeratorUtils.cpp consistent with the accepted
NS_ADDREF(*result), since I'm changing that file anyway
Attachment #101279 - Attachment is obsolete: true
ugh, so the monster list of files seems to have dropped all blank lines. so much
for my grep trick... 

Anyhow, I would really like to get this landed so consumers like PSM can start
using this and flush out more bugs/usability issues before we finally freeze
nsIArray. its getting very close to the 1.2 freeze and I want to give this as
much bake time as possible.
nsCOMArray(const nsCOMArray_base& aOther) : nsCOMArray_base(aOther) { }

this constuctor looks a bit dangerous to me. Won't that allow doing things like

nsCOMArray<nsIFoo> a;
...
nsCOMArray<nsIBar> b(a);

which will almost certainly break
review comments:

1- nsIArray.idl does not need to #include "nsISimpleEnumerator.idl" .. a forward
decl should suffice.

2- would it make sense to break nsICollection up into nsICollection and
nsIMutableCollection in order to make nsIArray and nsIMutableArray inherit from
those interfaces?  seems like the only real difference between an "array" and a
"collection" should be that an array can be randomly accessed efficiently.

more review comments to follow when i have more time :)
I spent a little bit of time looking at nsISupportsArrays that were created at
startup, and tried replacing some with nsCOMArray<T>. You'll see in this patch
that one big improvement is that we're doing a whole lot less AddRef/Releases -
especially in places where performance counts, like thread event processing and
widget child traversal. I'm sure there are lots more cases like this.
here's an updated "patch" of all the files.. reviews?
Attachment #101283 - Attachment is obsolete: true
The latest patch, together with the checked in files, seems to work.
I've attached a patch to bug 165574, which makes most of the crypto code switch
from nsISupportsArray to nsIArray/nsIMutableArray.
Comment on attachment 101577 [details] [diff] [review]
all files in one "patch"

In nsArray.cpp,
Check for a null parameters in GetLength, QueryElementAt, ect.

in nsArray::AppendElement, InsertElementAt, you can avoid an nsCOMPtr
assignment if you move the AppendObject into the branch.  Not sure if it really
matters.


in nsArrayEmumerator.cpp:
Do you want to add an assertion here:
// mValueArray[(mIndex-1)] = nsnull;

in NS_NewArrayEnumerator, I believe that you can optimize this:

*aResult = enumerator;
NS_ADDREF(*aResult);

into 
NS_ADDREF(*aResult = enumerator);

with that r=dougt
Attachment #101577 - Flags: review+
Comment on attachment 101554 [details] [diff] [review]
a few sample conversions from nsISupportsArray to nsCOMArray

r=dougt
Attachment #101554 - Flags: review+
Comment on attachment 101284 [details] [diff] [review]
add nsCOMArray, nsArray, and nsIArray to the builds v1.21

this would add the nsIArray.idl to the SDK idl's.  Did we *really* want to put
"UNDER REVIEW" idl's in the SDK?
Attachment #101284 - Flags: needs-work+
ok, here's the updated one with nsIArray.idl in XPIDLSRCS instead.
Attachment #101284 - Attachment is obsolete: true
Comment on attachment 101766 [details] [diff] [review]
add nsCOMArray, nsArray, and nsIArray to the builds v1.22

over AIM, I got r=dougt with the move
Attachment #101766 - Flags: review+
Attachment #101766 - Flags: superreview+
Comment on attachment 101766 [details] [diff] [review]
add nsCOMArray, nsArray, and nsIArray to the builds v1.22

sr=darin
ok, this has landed. I'll do the "sample" conversion work in another bug.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
i would probably be a good idea to make ObjectAt and operator[] return
nsDerivedSafe instead of raw pointers. That would give the nice protection
against doing NS_ADDREF(myArray[1]) that we have on nsCOMPtrs
ooh, that would be good. Do you know how to do that? I don't know how
nsDerivedSafe works...
basically, just declare the return type to be |nsDerivedSafe<T>|... no physical
work is involved.  Note: I haven't looked into your stuff yet, just replying to
your last comment. I will examine your patch soon.
do we have any users of nsCOMArray in the tree? i couldn't find any so i havn't
done any more testing then making sure that mozilla/xpcom still builds
Comment on attachment 102381 [details] [diff] [review]
use nsDerivedSafe rather then raw pointers

see attachment 101554 [details] [diff] [review] for some easy samples or bug 172004 for a big ugly patch
which starts using nsCOMArray
Darn, I'd have loved to be involved in the design of this before it went in -
but from what I can see (without having looked at the code yet), I like it. 
nsSupportsArray is a total monster, and is annoyingly inconsistent with
nsVoidArray).  I'll look at this tomorrow, but great work, Alec!
well, its still UNDER_REVIEW so there's always the possiblity of changing it.
We've switch at least one consumer over so far (PSM, for freezing purposes)
Some comments:

// index of the element in question.. does NOT refcount

You should note in the docs of the method that if the same object is in the
array twice, this will return the index of the first entry.  Not that this will
be a common usage, but it shouldn't be undefined.  You do mention the issue of
multiple copies of an object in the array for RemoveObject.

I like that on copy creator you use SizeTo/ReplaceObjectAt.

Overall, I can't find anything else obvious to add.  I like that it's
well-commented.  I'll look at a few "improve nsVoidArray/nsSupportsArray" bugs I
filed when I was reworking nsVoidArray to see if there's anything more.


In the conversion examples, I see a bug:

@@ -579,14 +576,9 @@ CSSLoaderImpl::RecycleParser(nsICSSParse
   nsresult result = NS_ERROR_NULL_POINTER;
 
   if (aParser) {
-    if (! mParsers) {
-      result = NS_NewISupportsArray(&mParsers);
-    }
-    if (mParsers) {
-      result = mParsers->AppendElement(aParser);
-    }
+    result = mParsers.AppendObject(aParser);
   }
-  return result;
+  return result ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
 }

This will return NS_OK if aParser is null.....

alecf: could you have a look at attachment 102381 [details] [diff] [review] now that we have some uses of
nsCOMArray in the tree?
Attachment #102381 - Flags: superreview?(alecf)
Attachment #102381 - Flags: review?(bzbarsky)
Comment on attachment 102381 [details] [diff] [review]
use nsDerivedSafe rather then raw pointers

looks good - at long as this doesn't prevent anything like the use of raw
pointers (i.e. nsIFoo* foo = foolist[i];) then I'm happy...
Attachment #102381 - Flags: superreview?(alecf) → superreview+
Comment on attachment 102381 [details] [diff] [review]
use nsDerivedSafe rather then raw pointers

sounds good.
Attachment #102381 - Flags: review?(bzbarsky) → review+
Comment on attachment 102381 [details] [diff] [review]
use nsDerivedSafe rather then raw pointers

Checked in. I had to change

NS_IF_ADDREF(aOther.ObjectAt(i))
to
NS_IF_ADDREF(NS_STATIC_CAST(nsISupports*, aObjects.mArray[i]));

In nsCOMArray_base::InsertObjectsAt
Attachment #102381 - Attachment is obsolete: true
Comment on attachment 102381 [details] [diff] [review]
use nsDerivedSafe rather then raw pointers

I want to back out this patch.	See bug 221525.
dbaron: Do you intend to remove nsIArray? The trouble is, nsIArray is already
being used in frozen interfaces.
no, he just means the latest patch to nsCOMArray to use nsDerviedSave
You need to log in before you can comment on or make changes to this bug.