407444 - Count of error-handling pattern "if (NS_FAILED(rv)) return rv;"

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

2151 is the count of "if (NS_FAILED(rv)) return rv" constructs over the Firefox browser trunk as compiled on Linux. Note that this count refers to the number of 'rv' variables referenced in such constructs. Thus, when an rv variable is reused in 2 more more of this pattern, it has only been counted once.

This is my very first use of DeHydra on Firefox so you should probably take the number with a grain of salt. I did spot-check about 5 cpp files to verify that the script found the correct number of patterns in that file.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: List of source code locations of patterns found

Comment 2

[Jesse Ruderman]

What is the purpose of this bug?  Is this pattern bad?

Comment 3

[David Mandelin [:dmandelin]]

It's a measurement that was wanted for work on using C++ exceptions in Moz2.0. I was asked to post the results to bugzilla.

Comment 4

[Benjamin Smedberg]

Are you sure? NS_ENSURE_SUCCESS(rv, rv) is this pattern in release builds (it adds a warning in debug builds), and there are many many of those: http://mxr.mozilla.org/mozilla/search?string=ns_ensure_success(rv%2C%20rv)

Comment 5

[David Mandelin [:dmandelin]]

bsmedberg: Thanks for the tip. I wasn't looking for !NS_SUCCEEDED so I missed all the NS_ENSURE_SUCCESS. I have corrected that problem, and the new count is 10955. Does that sound more likely? I'm attaching the new list.

The fix also picks up patterns like "if (NS_SUCCEEDED(rv)) { do_stuff; } return rv;" That seems right. If not, let me know.

I have a better idea of the limitations of this version of the analysis. In particular, it won't pick up this pattern:

if (NS_SUCCEEDED(rv) && other_cond) {
  do_stuff;
} else {
  return rv;
}

I don't that's *too* common, but it probably does occur.

Comment 6

[Brendan Eich [:brendan]]

We could use a style checker, since

if (NS_SUCCEEDED(rv) && other_cond) {
  do_stuff;
} else {
  return rv;
}
more_stuff...

is bad style. In general, else after return/break/continue/goto is poor style leading to overindented "normal-case" code. A checker would look for such non-sequiturs as well as the above, which is in general if(A){B}else{C;return}D and should be if(!A){C;return}B;D.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> The fix also picks up patterns like "if (NS_SUCCEEDED(rv)) { do_stuff; } return
> rv;" That seems right. If not, let me know.

So this too is poor style compared to if (NS_FAILED(rv)) return rv; do_stuff; return NS_OK (or rv; NS_OK might be cheaper to synthesize that homing and reloading rv). We'd like to count this with a separate counter. In general, split out counts and sum the related ones that pass exceptions through, so we can see both the style nits to fix and the totals.

/be

Comment 8

[David Mandelin [:dmandelin]]

OK, I'm working on the style checks. In the meantime, I now have stats on the other patterns, with the same caveats as before, as well as another one I just realized: constructs like 'return NS_FAILED(rv) ? foo : bar' are not, to my knowledge, recognized as control flow by Dehydra. The stats are:

(1) cascade               10999
(2) cascade-after-cleanup  1652
(3) catch-and-continue     1809
(4) catch-and-rethrow      2862

(1) is 'if (NS_FAILED(rv)) return rv'
(2) is 'if (NS_FAILED(rv)) { foo; return rv; }'
(3) is 'if (NS_FAILED(rv)) { foo; } bar; return 0;'
(4) is 'if (NS_FAILED(rv)) { foo; return other_error_code; }'

Note that at least a few instances are in multiple categories, e.g., because there is both a path to return rv and a path to return 0.

As a sanity check, the text NS_ENSURE_SUCCESS occurs about 7700 times in the codebase (including dups, headers, and comments). These are hopefully all counted in the cascade category. NS_FAILED occurs 13600 times and NS_SUCCEEDED 6059 times. I would expect a lot of the NS_SUCCEEDED not to be counted in the current version, but even leaving them out, it seems some NS_FAILED may be missing, although my spot checks haven't found any such lost occurrences yet.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does 3 include code that completely ignores the returned rv value and just goes on assuming everything was alright?

Comment 10

[David Mandelin [:dmandelin]]

(In reply to comment #9)
> Does 3 include code that completely ignores the returned rv value and just goes
> on assuming everything was alright?

No, although I was thinking we probably want to count those as well. So far, I've been looking at what is reached from NS_FAILED and NS_SUCCEEDED. Finding the ignore case would require figuring out how to recognize methods that return nsresult and tracing from those. 

I was also thinking that methods that have return type nsresult but in fact never fail and always return 0 are a different category: with exceptions, they require no error checking or exception handling. On the other hand, methods that now ignore nsresults for methods that do fail would get a do-nothing catch block (in a semantics-preserving refactoring to exceptions, anyway).

Comment 11

[David Mandelin [:dmandelin]]

I implemented the style checker suggested by Brendan, checking for 2 patterns. It is set up right now to detect these patterns only when the condition refers to NS_FAILED or NS_SUCCEEDED.

// return immediately after if/then
if (...) {
  stuff;
}
return rv;

// else only ends in return
if (...) {
  stuff;
} else {
  other;
  return rv;
}

I'm posting the list as an attachment.

Note that this pattern is also included in the results, although it doesn't seem as bad. I can take it out if desired.

if (...) {
  stuff;
  rv = foo();
}
return rv;

Comment 12

[David Mandelin [:dmandelin]]

Attachment: list of 'overindented' NS_FAILED/SUCC test patterns

Comment 13

[David Mandelin [:dmandelin]]

Attachment: ignored nsresult return values

Here's a preliminary list of call sites that ignore nsresult return values. There are 10413 of them. This version of the analysis isn't super-precise, so it should be considered a rough estimate.

The potential significance of a call site that ignores an nsresult return value is that it would have to be wrapped in 'try { call_foo(); } catch { }' if using exceptions. But there are any number of reasons why not all call sites that ignore an nsresult have to be wrapped that way. Example 1: if the the method called never throws an exception (always returns NS_OK in current code). In this case, there would be no catch block. Example 2: the caller performs some other test that reliably checks for failure (e.g., testing if a getter returned NULL). If all callers use the same test, then the function could be rewritten not to use an exception. 

In order to get a handle on these issues, I'm going to investigate and classify a small sample by hand.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: ignored nsresult return values

This is a new list of calls with ignored return values. This analysis should be much better than the previous one. This time there are about 20,000 sites flagged. Now I'll look at a sample in detail and see what I get.

Comment 15

[David Mandelin [:dmandelin]]

I just completed inspection of 20 ignored return values in /layout. I classified them as follows:

  11 nothrow       function always returns NS_OK
   3 scriptable    function is scriptable
   3 nofail        function will not fail at this call site
   2 nullcheck     caller checks for null outparam
   1 acceptfail    caller runs OK if call fails

Some notes:

- For the scriptable methods, all the C++ implementations always return NS_OK.

- One of the nofail methods is only used in one .cpp file at two places. It appears this method could be rewritten as nothrow. (nsStyleContent::SetCounterResetAt)

Comment 16

[David Mandelin [:dmandelin]]

Attachment: List of methods that always return NS_OK

Here's a list of methods found to always return NS_OK by a simple static analysis. There are about 10,000 of them, out of 30,000 nsresult-returning methods in all. The results agree with the by-hand analysis I did on the sample of 20 (with the exception that the analysis caught one error I had made in thinking something was always NS_OK when it really wasn't). Caveats:

- Of course, this analysis makes no attempt to address the issue of methods implemented in JS: the analysis only looks at C++ method implementations. I could filter the results if someone has a list of the methods that are actually implemented in JS.

- The intraprocedural analysis is flow-insensitive, and the interprocedural analysis uses a type-based call graph, so there may be a few other always NS_OK methods that I missed. But I doubt there are very many: the interprocedural analysis only catches 300 more always-NS_OK methods than the intraprocedural analysis.

- There are some template methods, like CallQueryInterface<X>, that my analysis doesn't look into, and assumes can return an error.

Next I'll classify the ignored nsresult call sites I found earlier by whether that call site only hits methods from my always-NS_OK list.

Comment 17

[David Mandelin [:dmandelin]]

Attachment: Ignored nsresults of methods that can return non-NS_OK values

Here is the list of call sites that ignore nsresults filtered to only those where a possible call target really can return something other than NS_OK. This is about 2/3 of all call sites that ignore nsresults.

I analyzed a sample of 20 in detail (see sample.txt in the attachment). In all 20, the analysis was correct in finding that a target method can return a non-NS_OK result (modulo the conservative call graph). But 1 was a false positive because the nsresult wasn't actually ignored, but was stored in a field for later testing, which my ignored nsresult analysis doesn't look for.

Of the other 19, the breakdown is:

    6 acceptfail -- the caller doesn't need to do anything special for errors
    2 nofail -- at this particular call site, the return is always NS_OK
    5 unknown -- hard for me to analyze, but probably acceptfail or nofail
    3 nullcheck -- caller checks for failure by testing if an outparam is null
    1 boolcheck -- caller checks as if the result is a PRBool, which it
                   really was: appears to be a bug in callee declaration
    2 bug -- minor bugs. One would crash a Dogbert profile import on OOM, the other returns NS_OK when it doesn't mean to, although it also leaves an outparam string empty, so it's probably OK.