130544 - Anomaly in return value of AppendElement method in nsSupportsArray.h

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P3)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

This is an interesting one.

in nsSupportsArray.h, the method AppendElement should return a nsresult but
instead returns the result of nsSupportsArray::InsertElementAt, which is a
PRBool. That seems wrong.

Some historical notes:

- on April 13, 1999, it looks like Warren checked in a change to make the
AppendElement method return NS_OK or NS_ERROR_FAILURE based on the result of
InsertElementAt
- on April 14, 1999, Chris Waterson backed out this change.
- on December 20, 2001, Randell Jesup adds a comment suggesting the return of
nsresult values depending on the return value of InsertElementAt

What's the deal here? Having functions that claim to return nsresults and
actually return PRBools is confusing and could cause bugs. None of the 1999
checkins reference bug #s so I can't tell what the effects of warren's change
was or why it was backed out.

Comment 1

[Brendan Eich [:brendan]]

What goes wrong if we try to fix this?  It must be that when something returns
false, warren's change causes NS_ERROR_FAILURE to be returned -- which is
failure and defeats something that works when 0 (PR_FALSE, NS_OK) is returned
instead of 1 (PR_TRUE, NS_COMFALSE) is returned.  I don't see how that can
happen for AppendElement -- malloc failure should be failure, not success.

So, by deduction, the problem must have been RemoveElement's return value.  That
makes sense.  It's layered on RemoveElementsAt, which returns false if the index
is >= the array length.  So the solution here is two-fold:

1.  Fix AppendElement to return NS_OK on success, NS_ERROR_OUT_OF_MEMORY on failure.

2.  Fix NS_IMETHOD RemoveElement(nsISupports *aElement) to be infallible -- it
should always return NS_OK, not false (NS_OK) on index out of bounds and true
(NS_COMFALSE!) on index in bounds.

Patch in a trice!

/be

Comment 2

[Brendan Eich [:brendan]]

Oh, wait: InsertElementAt overloads its false return value to mean two very
different things: out of memory, and index out of bounds.  I'm sticking with
NS_ERROR_FAILURE for the fixed failure result code.

/be

Comment 3

[Brendan Eich [:brendan]]

Oh crap, here we go again: I can't disambiguate false (meaning OOM) from false
(meaning index out of bounds) from InsertElementAt's return in AppendElement. 
This truly sucks.  I'm inclined to make AppendElement infallible too.

/be

Comment 4

[Brendan Eich [:brendan]]

Can you believe it?  When I wrote "index out of bounds" in the last comment, I
should written "index > array length" -- an aIndex == mCount is valid as an
insertion point, and the code grows the array if needed.  So unless I feel very
lucky, some code probably counts on inserting *one beyond* the end of the array
returning false, i.e. NS_OK, from AppendElement!

/be

Comment 5

[Brendan Eich [:brendan]]

Boy, I need caffeine.  Ignore my ramblings.  The InsertElementAt API is horribly
broken, overloading false as it does.  But, AppendElement is ok, because it
calls InsertElementAt(aElement, mCount), and that can fail only due to OOM. 
Patch now.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix

Looking for r= and sr= for 1.0 -- dougt, can you r=?

/be

Comment 7

[Brendan Eich [:brendan]]

I stuck with NS_ERROR_FAILURE, even thought we know OOM is the only possible
cause of failure for AppendElement's call to InsertElementAt(aElement, mCount)
-- comments?

/be

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 73950 [details] [diff] [review]
proposed fix

r=dougt.  nsSupportsArray is sucky.  See 91362

Comment 9

[John Bandhauer]

Hello? How can you contemplate changing this without finding and fixing *all*
callers who check the result? I know *I've* written code that takes into account
that the return here is really a boolean. I can't be the only one. This change
by itself will cause some number of callers to break, no?

Comment 10

[Brendan Eich [:brendan]]

Well, *I've* written code that uses the nsresult promised by nsICollection.idl.
 So there, nyah. ;-)

Seriously, jband: why did you hack implementation-specific bug knowledge into
your uses?  I'm lxring now, I'll try to capture all the current uses.  I still
think we should fix this, or we'll never fix it, and we'll have to go find all
the places that follow the interface, not the impl.

/be

Comment 11

[Doug Turner (:dougt)]

Why does this have to be fixed for mozilla 1.0?  Why do we care about this right
now?

Comment 12

[John Bandhauer]

brendan: I have this odd desire to check result codes and handle failures. This
coupled with my wanting my callers to not fail just because the implmentation
does not match the documentation/declaration has apparently got me in a bind.

I suppose it would be more disruptive to make the nsICollection methods use
"[notxpcom] boolean" like the nsISupportsArray methods?

Comment 13

[Brendan Eich [:brendan]]

jband, I share your desire to check results, hence
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsFastLoadFile.cpp#759.  But my
desire to check results does not cause me to check for broken implementations;
life's too short, bugs come out in the wash, and they're not my bugs if some bad
old implementation "lied".

I don't know if [notxpcom] boolean will go over well at this point.  What do the
rest of the cc: victims think?

/be

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

I believe I tried to change it and things broke, so I backed off on the API
change.  Yes it's evil.  Yes it's good to check return codes; I seriously
dislike people not checked allocation returns (even indirect ones).  Even if a
function can't fail today, often it can tomorrow if the internal implementation
changes.  Doubly so for XPCOMed API's.

Comment 15

[Doug Turner (:dougt)]

over to brendan who is working on this

Comment 16

[Brendan Eich [:brendan]]

Ok, for the patch I'm attaching, which is preliminary, I'll need mailnews guys
to r= or demand a smaller change.  A few weeks ago I actually made the .idl file
change jband suggested ([notxpcom] boolean for appendElement and removeElement
return types), rebuilt, ran into trouble in mailnews/base/util/nsMsgFolder.cpp,
went crazy fixing indentation there along with actually checking the boolean
results, and forgot to update this bug.

I'm still rebuilding, not sure how much else might need to change in order to
compile.  I'm not proposing to change every call that currently treats the
return value as an nsresult to treat it as a boolean, for either method.  We
have code today that will never notice failure (because 0x80000000 isn't set in
the r.v.) and I'd like to leave that code broken in that way, but check in the
.idl fixes and the nsMsgFolder.cpp cleanup for 1.0.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: tentative fix

Comments and testing help welcome.

/be

Comment 18

[David :Bienvenu]

I've looked through the patch a bit. A few comments:

We tend to use NS_ENSURE_ARG_POINTER in mailnews to check for null argument
pointers so I think using that would be more stylistically consistent.

I really don't like NS_ERROR_FAILURE since it's so hard to find where it come from:

 	nsCOMPtr<nsIFolder> folder(do_QueryInterface(res, &rv));
 	if (NS_FAILED(rv))
 
	return rv;
-
if (aFolder)
-
{
+  if (!aFolder)
+    return NS_ERROR_FAILURE;
+
but I think this will never ever happen (famous last words) so it doesn't matter.

The patch looks OK - is there any potential for problems with this change? Is it
just a style/preferred practice thing? 

-
	nsCOMPtr<nsISupports> supports = getter_AddRefs(mSubFolders->ElementAt(i));
+      nsCOMPtr<nsISupports> supports(do_QueryElementAt(mSubFolders, i));

Comment 19

[timeless]

if you weren't going to nsISupports, it'd be a performance thing since you'd 
double QI.  So I guess that technically it's style, but afaik only because your 
nsCOMPtr is nsISupports. 

1678                    nsCOMPtr<nsIMsgFolder> folder;

I think brendan wanted:
-               nsCOMPtr<nsISupports> supports = 
getter_AddRefs(mSubFolders->ElementAt(i));
-               folder = do_QueryInterface(supports, &rv);
+      folder = do_QueryElementAt(mSubFolders, i));

Comment 20

[Brendan Eich [:brendan]]

scc said a while ago that copy-constructor style init was faster on some C++
setups than assignment style init.  Cc'ing him and dbaron for the latest.

How about NS_ERROR_UNEXPECTED instead of ...FAILURE?

I'd like another mailnews guy to have a look at the nsMsgFolder.cpp patch, if
you're feeling able-to-generous.  Thanks,

/be

Comment 21

[David :Bienvenu]

NS_ERROR_UNEXPECTED is better because we don't use it as much :-) - in future, I
think we should have an error code that means QI_FAILED_THAT_SHOULD_NEVER_FAIL :-)

Comment 22

[Brendan Eich [:brendan]]

Forgot to note that do_QueryElementAt is better because leak-proof, compared to
calling ElementAt without a getter_AddRefs or dont_AddRef around the call -- no
such problem in nsMsgFolder.cpp, but that kind of leak has happened elsewhere
and I'm trying to promote the leak-proof pattern, so we never have to worry.

/be

Comment 23

[Aleksey Nogin]

Dup of 48783?

Comment 24

[Brendan Eich [:brendan]]

I'll attach an updated patch later today.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: updated trunk patch (diff -wu)

Ok, I'd like to get this in for 1.1alpha and then 1.0.1 when the 1.0 branch
opens up for that release.  Can I get r= and sr=?

/be

Comment 26

[David :Bienvenu]

Comment on attachment 85196 [details] [diff] [review]
updated trunk patch (diff -wu)

sr=bienvenu, as is, thx, Brendan.

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 85196 [details] [diff] [review]
updated trunk patch (diff -wu)

fix up the ws for sure.

Do you wanna rename RemoveElement to RemoveFirstElement?  Or maybe just fix the
comment up:

// XXX this is badly named - should be RemoveFirstElement

r=dougt

Comment 28

[Brendan Eich [:brendan]]

Fixed on trunk.  Going to drivers for 1.0 branch approval for 1.0.1 first
release (should avoid merge headaches).

/be

Comment 29

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP, and replace the mozilla1.0.1+ keyword
with fixed1.0.1 once it's landed there.

Comment 30

[Brendan Eich [:brendan]]

Fixed in the 1.0 branch too.

/be

Comment 31

[Brendan Eich [:brendan]]

D'oh: bug 147997.

/be

Comment 32

[Brendan Eich [:brendan]]

I give up.

/be