Proposal for change in standard anti-crash C++ checks

Kent James kent at caspia.com
Wed Nov 6 00:18:48 UTC 2013


I'd like to have a small discussion about standard ways of checking for 
errors in C++ code as anti-crash prevention, and propose two code standards.

1) A common pattern in mailnews code is the following:

   nsCOMPtr<nsIMsgIncomingServer> server;
   nsresult rv = GetServer(getter_AddRefs(server));
   NS_ENSURE_SUCCESS(rv, rv);
   return server->GetMsgStore(aStore);

The problem with that is that sometimes we may have code that 
intentionally returns a null value, as is being proposed in bug 739908 
for spamSettings. Nobody really knows which code follows which standard, 
so these checks are always risky.

I would like to propose that, instead, the pattern be the following when 
all we are doing is preventing crashes, but have every expectation that 
the getter should succeed:

   nsCOMPtr<nsIMsgIncomingServer> server;
   nsresult rv = GetServer(getter_AddRefs(server));
   NS_ENSURE_STATE(server);
   return server->GetMsgStore(aStore);

This code returns NS_ERROR_UNEXPECTED if the server is null when we are 
expecting it to be valid.

This has the advantage of 1) it is safer, and 2) it is shorter. It has 
the disadvantage that the rv is not propagated. So if the error is 
reasonably
expected, such that the upstream code might choose to act depending on 
the rv value, you would not follow this pattern. Instead, you would have 
to follow:

   nsCOMPtr<nsIMsgIncomingServer> server;
   nsresult rv = GetServer(getter_AddRefs(server));
   NS_ENSURE_TRUE(server && NS_SUCCEEDED(rv), NS_FAILED(rv) ? rv : 
NS_ERROR_FAILURE);

That line is long enough and common enough that it would be worth a 
standard macro, something like:
   NS_ENSURE_PTR_SUCCESS(rv, server);

2) As a related standard, all code entering a C++ method has checked 
pointers. (You do not need to check for null before sending a ptr to a 
method).

Neil objected to this requirement here: 
https://bugzilla.mozilla.org/show_bug.cgi?id=522886#c110 with the comment:

"(not needed) because this is an internal method and the callers already 
null-check their variables before passing them in"

I think that the standard should be that if your method would crash with 
a null ptr, you must check the value to ensure it is non-null before 
using it.

Comments?

(And sorry for the early send. Who decided that ctl-Enter sends the 
message instead of adding a line feed ???)



More information about the tb-planning mailing list