Anomaly in return value of AppendElement method in nsSupportsArray.h

RESOLVED WONTFIX

Status

()

P3
normal
RESOLVED WONTFIX
17 years ago
17 years ago

People

(Reporter: bugs, Assigned: brendan)

Tracking

({arch})

Trunk
mozilla1.0.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

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.

Updated

17 years ago
Keywords: arch
(Assignee)

Comment 1

17 years ago
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
Keywords: mozilla1.0
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

17 years ago
Created attachment 73950 [details] [diff] [review]
proposed fix

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

/be
(Assignee)

Comment 7

17 years ago
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 on attachment 73950 [details] [diff] [review]
proposed fix

r=dougt.  nsSupportsArray is sucky.  See 91362

Comment 9

17 years ago
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?
(Assignee)

Comment 10

17 years ago
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
Why does this have to be fixed for mozilla 1.0?  Why do we care about this right
now?

Comment 12

17 years ago
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?
(Assignee)

Comment 13

17 years ago
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
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.
over to brendan who is working on this
Assignee: dougt → brendan
(Assignee)

Comment 16

17 years ago
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
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 17

17 years ago
Created attachment 76938 [details] [diff] [review]
tentative fix

Comments and testing help welcome.

/be
Attachment #73950 - Attachment is obsolete: true

Comment 18

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

17 years ago
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));
(Assignee)

Comment 20

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

17 years ago
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 :-)
(Assignee)

Comment 22

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

17 years ago
Dup of 48783?
(Assignee)

Comment 24

17 years ago
I'll attach an updated patch later today.

/be
Keywords: mozilla1.0 → mozilla1.0.1
Target Milestone: mozilla1.0 → mozilla1.0.1
(Assignee)

Comment 25

17 years ago
Created attachment 85196 [details] [diff] [review]
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

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

sr=bienvenu, as is, thx, Brendan.
Attachment #85196 - Flags: superreview+
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

Updated

17 years ago
Attachment #85196 - Flags: review+
(Assignee)

Comment 28

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

/be

Comment 29

17 years ago
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.
Keywords: mozilla1.0.1 → mozilla1.0.1+

Updated

17 years ago
Attachment #85196 - Flags: approval+
(Assignee)

Updated

17 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1
Resolution: --- → FIXED
(Assignee)

Comment 30

17 years ago
Fixed in the 1.0 branch too.

/be
(Assignee)

Comment 31

17 years ago
D'oh: bug 147997.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 32

17 years ago
I give up.

/be
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.