778110 - nsSupportsArray::AppendElement/RemoveElement treat bool as nsresult

Status: NEW

(Core :: XPCOM, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

These are supposed to return nsresult, but actually return the result of InsertElementAt/RemoveElement, which return bool.  This is probably equivalent to just always returning NS_OK, but might not be, if there are any callers that are treating the value as a boolean (the mistakes might cancel out!).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes, this is a known "feature" of supports array.  Callers do in fact know to treat this as a boolean, generally.

The right solution is getting rid of nsSupportsArray.  ;)

Comment 2

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> The right solution is getting rid of nsSupportsArray.  ;)

And replace it with what?  Regular arrays?

This will be a somewhat annoying project considering the fact that there are JS consumers of nsSupportsArray, and the dependency spans across m-c and c-c.  :(

Can't we just cast our way out of this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> And replace it with what?  Regular arrays?

TArray/nsIArray/C++[] depending on application.  This is a long-term project, obviously...  I wasn't suggesting it be done in this bug; hence the smiley.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Yes, this is a known "feature" of supports array.  Callers do in fact know
> to treat this as a boolean, generally.
> 
> The right solution is getting rid of nsSupportsArray.  ;)

Is this bug the only reason to ditch nsSupportsArray, or are there other footguns/suboptimalities that make is undesirable compared to the alternatives?

Comment 5

[Boris Zbarsky [:bzbarsky]]

There are all sorts of footguns.  Starting with the ElementAt() method on it!

I thought we had a bug about removing this stuff.  We certainly have a (fixed) bug on deprecating it....