802473 - NS_FAILED returns values other than 0 and 1; NS_FAILED/NS_SUCCEEDED should use branch prediction macros

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mats Palmgren (inactive)]

Bug 768865 removed "defined(NS_STATIC_CHECKING)" so that a faulty
version of NS_FAILED is now used for all C++ code.
There are two problems:

1. Assigning NS_FAILED(an_error) to an integral type gives the wrong result.
   Assigning to a variable of narrower type results in zero, assigning to a
   same or wider type it's non-zero but still wrong (0x80000000).
   The result should be 1, as documented in the file.

2. Since the result of NS_FAILED/NS_SUCCEEDED is very likely to be 0/1
   we should use NS_LIKELY/NS_UNLIKELY to indicate so.  I'm surprised
   bug 768865 didn't cause a performance regression, but perhaps PGO
   wallpapers over it?

Since NS_LIKELY/NS_UNLIKELY returns exactly 0 or 1, we get point 1 for free.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

This fixes point 1 and 2 above, and still keeps the desirable
type check that bug 768865 was after, IIUC.

It appears to be green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=d715a0990034

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 672149 [details] [diff] [review]
fix

>-inline int NS_FAILED(nsresult _nsresult) {
>+inline int _NS_FAILED(nsresult _nsresult) {
Don't use underscore + uppercase. It is always reserved in C/C++.

Comment 3

[(no longer active)]

Comment on attachment 672149 [details] [diff] [review]
fix

Review of attachment 672149 [details] [diff] [review]:
-----------------------------------------------------------------

stealing the review.  Please call the underlying function/macro something like NS_FAILED_impl.

Comment 4

[(no longer active)]

Also, out of curiosity, how did you find out about this?

Comment 5

[Benjamin Smedberg]

Why is this an int and not a bool return value?

Comment 6

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

It shouldn't be.  I thought I changed it to bool in some patch of mine somewhere along the line, but it seems I didn't.

Comment 7

[Benjamin Smedberg]

ok, can you make them bools?

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix, v2

> Please call the underlying function/macro something like NS_FAILED_impl.

Fixed.

> ok, can you make them bools?

Fixed.  NS_FAILED_impl isn't supposed to be called directly so I made
it return uint32_t to avoid any unnecessary cycles spent before giving
the value to NS_LIKELY.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Also, out of curiosity, how did you find out about this?

The background is that I investigated this exact problem with assigning
the result of NS_FAILED in bug 340244 a few years back.  It caused a test
to fail because of an assignment to a PRUint8 (see bug 340244 comment 5
for all the gory details).

Then I stumbled over bug 801464 and got curious what NS_FAILED looked like
nowadays after the nsresult enum changes, and when I saw it I knew instantly
that it was broken.

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9871b40e0f6

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9871b40e0f6