11596 - [WebShell] eliminate NS_COMFALSE

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Warren Harris]

We should eliminate the NS_COMFALSE madness once and for all. Here's a list of
offending uses in your module -- please pass the bug along if there's someone
else who should deal with it.

      webshell/src/nsWebShell.cpp:
            View change log or Blame annotations
            line 2439
            line 2444
            line 2467
            line 2472

Comment 1

[Warren Harris]

Things that could have returned NS_COMFALSE in the past should instead have an
out parameter that's a PRBool. Jband was going to post a canonical example
somewhere (I think).

Comment 2

[John Bandhauer]

There is discussion in http://bugzilla.mozilla.org/show_bug.cgi?id=11598

Comment 3

[Nisheeth Ranjan]

CCing Scott Collins, new webshell owner...

Accepting bug and setting milestone to M10..

Comment 4

[Nisheeth Ranjan]

Bul re-assigning webshell bugs to the new webshell owner, Scott Collins.  Scott,
please adjust the target milestone assignments as you see fit.  Thanks.

Comment 5

[Scott Collins]

(Mass-) Re-assigning [Webshell] bugs to travis, on the advice of dp, et al.  Many
of these may be invalid in the new world of the re-written webshell, but he
certainly needs to be aware of these issues.  Hope this helps you, travis.

Comment 6

[travis]

I don't think this is valid.  NS_COMFALSE is perfectly acceptable where used
correctly....  New design will use it where it is required and will document.
If there is a problem on a specific method then, open a bug on that method.
This particular bug is being marked invalid.

Comment 7

[Warren Harris]

Travis, We've found use of NS_COMFALSE to be problematic for several reasons --
the primary one is interaction with xpconnect, but also a lot of pilot errors.
This bug is here to make sure that we eradicate it from our code.

Comment 8

[Brendan Eich [:brendan]]

I'm sorry, NS_COMFALSE is going away and should not be used.  This has been
hashed out in the xpcom newsgroup (search for COMFALSE), and in bugzilla (see
the dependent bug, 8929, and its dependencies).

Overloading a status or result code return value tends to produce code that
checks for only one of three or more possible result codes, hiding errors and
treating them as true or false by misimplication.  This has happened and is
still happening in our tree; even top hackers make the mistake.

What's more, XPConnect and its Java, Perl, etc. siblings are not going to
special-case NS_COMFALSE, leading to incredibly contorted code (see rev 3.2 of
xpcom/components/nsCategoryManager.js).

To avoid both the overloaded-logic hazard and the XPConnect-related complexity,
we had decided to get rid of NS_COMFALSE.  It was a premature optimization given
boolean out params in C++.

Close this bug if you like, but please don't use NS_COMFALSE -- it is going away
as soon as possible, given other priorities.  Of course (as you know well), if
you use XPIDL to declare boolean methods, then you won't have to worry about
NS_COMFALSE at all.

/be

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I have yet to see a case where _correct_ use of NS_COMFALSE is significantly
simpler than use of a boolean out, though I've seen lots of places where
incorrect use is more convenient.

I direct the eyes of the jury to
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsEnumeratorUtils.cpp#159 .
This code is designed to hide the evil of NS_COMFALSE from the world, but which
_itself_ misuses the interface.  Note how it assumes that any non-NS_OK return
indicates a lack of additional elements, when it should be properly checked for
|rv == NS_COMFALSE| (no more elements), |rv == NS_OK| (more elements) and
|NS_FAILED(rv)| (operation failed).  And if you're going to check
|NS_FAILED(rv)| and NS_COMFALSE individually, why not just use a boolean out?
(While we're at it, what does a non-NS_OK success code mean?)

Travis, can you please point me to a place in the Mozilla source where an
NS_COMFALSE-returning interface is used correctly, and explain why it's simpler
to use NS_COMFALSE than a boolean out?  I don't have very much COM experience
outside of Mozilla, but what I've seen within Mozilla has made me hate
NS_COMFALSE very deeply indeed.

Comment 10

[travis]

I still don't find this valid.  There are many cases where it is acceptable to
return success but would want to clarify some sort of sucess.  These are not
error conditions nor are these cases where you should be hurt by simply testing
success.
Cases where you would be hurt for not testing the exact return value don't just
live in success conditions but in nearly every single error condition in our
code.  if(NS_FAILED(rv = foo()) return rv; is absolutely WRONG.  That is every
where and is a much bigger problem.  To be proper, we should be checking return
results based on what the specific function said it will return.

Comment 11

[travis]

Well an example of how this can be used is (note it says NS_FALSE, it means
NS_COMFALSE).
http://lxr.mozilla.org/seamonkey/source/xpcom/appshell/eventloop/xp/nsIEventLoop
.idl#197.  But before that and after code in the interface you will see places
in the API that cleary state the return of NS_COMFALSE that would require
checking against the return value to get proper flow.

That said, we seem to say the solution is to add a boolean return value.  So
basically what we are saying is because people are failing to look at one built
in return value that we should add another one for them to potentially also
ignore?  The whole point of the standard returns in COM is that there is a
build in system for returning errors.  Simply because someone *incorrectly*
thought along the way we could blindly look at these errors as NS_SUCCESS or
NS_FAILED and all would be fine we have a problem with them?  At no point is it
valid to simply assume that all interfaces returning a success code means blind
sucess of everything.  And at no point is it valid to simply think all
interfaces return the same errors.

Interfaces are supposed to document their returns for a reason.  It is up to
the client to check each one of those return cases that it might care about.
If a client wishes to assume all errors within a given call would be bad for
it, then it can with NS_FAILED.  But if it needs to check a particular error
return, then it is it's responsibility to do so.

Comment 12

[Brendan Eich [:brendan]]

You can say "if only people followed the rules..." but some rules aren't worth
following.  There is no benefit to overloading the return value to convey false
or true as well as failure codes.  Yes, we need to check and reflect error codes
carefully, according to well-defined interfaces -- in fact, I think we should
declare error results in XPIDL so we can automate coverage tests.  But, if we
can have one fewer rule, and a simpler rule set, at no cost to XPIDL or XPCOM
expressiveness, why not simplify?

Even if people were perfect, I'd argue against COMFALSE and other "success"
code variations that encode NS_OK + anOutParameterValue.

/be

Comment 13

[travis]

But my point is, what we are saying is since they can't follow rule A.  Lets
create rule B that they maybe will follow?  In the end we are going to have to
have everyone following rule A.  We have already seen countless places where
people failed to follow the error checking rule or simply mis-used it.  This
has broken us numerous times.

So you then go on to say that we can't overload returns.  This seems to say you
want the return to be a black and white boolean yes or no did it fail.  This
then forces parameters to determine what kind of failure.  This is exactly why
COM has common return results.  Instead of forcing everyone to create numerous
error return paths, why not centralize them all through one standardized one.
So you then might say we do that but why should there be multiple success
types?  The answer is simple, sometimes your function calls aren't black and
white sucess or failure.  Sometimes you have different successes.  In your
system we must then create paramaters that allow the client to know what kind
of success.  Why are errors different than sucess?  We all agree that an error
isn't black and white, so why is a success supposed to be black and white?

There is a system setup for common *return* codes.  Some where along the way
for some reason someone started thinking this was only for *error* codes.  Well
it's not and you are supposed to check all *return* codes.  COM provides the
facility to allow some top-level grouping of these return codes.  This is the
distinction between success and error types.  But for any given call you have
to know if you can differentiate on a group level or must look at the exact
return code level.

You further state that fewer rules are easier.  I agree. But what you are doing
is adding rules.  The rules are simple now.  There is one and only one.  Check
all your return codes.  You are suggesting adding another rule.  Check these
set of return codes then check this other one etc.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

What is the type of operational success conveyed by NS_COMFALSE in
nsIEnumerator::isDone?

I believe that there is a fundamental difference between having varying types of
successful operation or failed operation, and encoding in the result code what
is conceptually a boolean out parameter.  Especially in cases where an
intervening layer (thread proxy, XPConnect) can affect the nsresult return.

I again offer the case of nsIEnumerator::isDone, which is declared in IDL as:
  void isDone();

That makes no sense.  This method is conveying information to the caller --
which is why the method name is conveyed evokes a statement of fact:
``if(enumer.isDone()) -> if this enumerator is Done'' -- and yet the
IDL-expressed interface contract gives no mention of that information, or how it
is conveyed.

Why would you _prefer_ NS_COMFALSE over a boolean out parameter?  That point is
still not clear to me.  I think that having the out parameter there makes it
much clearer, even in C++, and I don't see nearly as many misuses of boolean out
as of NS_COMFALSE.  Maybe that's just coincidence, though.

As far as friendliness to other languages, NS_COMFALSE is clearly inferior to
boolean-out.  Even looking at Microsoft's own COM-Java stuff, you have to catch
ComSuccessException to detect an S_FALSE result code, which is pretty horrible:
there's nothing exceptional about an enumerator having more elements.

Should we take this to the newsgroup?

Comment 15

[John Bandhauer]

Or just search the newsgroups for this discussion version 1.0

This is a dead snake. NS_COMFALSE is a bit of legacy that we don't need and most
of us don't want. If we need any discuassion it ought to be "how to get rid of
it" not "if to get rid of it". We have better things to argue about.

Comment 16

[leger]

Clearing INVALID resolution due to reopen of this bug.  But it sounds like this
is a Resolved/Won't Fix...yes?

Comment 17

[Brendan Eich [:brendan]]

NS_COMFALSE is going away before "architecture complete", so this should stay
open until its uses have gone.

/be

Comment 18

[travis]

Marking fixed.  None of those in WebShell...

Comment 19

[Chris Petersen]

Marking verified per last comments.