Closed Bug 1299384 Opened 3 years ago Closed 3 years ago

Use MOZ_MUST_USE with NS_warn_if_impl()

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 2 open bugs)

Details

(Keywords: coverity)

Attachments

(1 file, 2 obsolete files)

There are 1000s of NS_WARN_IF uses like this:

> if (NS_WARN_IF(NS_FAILED(rv) {
>   ...
> }

But there are also a couple of hundred uses where it's used outside a
condition, like this:

> NS_WARN_IF(NS_FAILED(rv));

The latter causes a problem. Coverity has a useful warning where it identifies
missing checks for functions whose return values are usually checked.
Unfortunately, we get many false positives because of NS_warn_if_impl().

There's already a suggestion of how to deal with this in the codebase. About a
quarter of the just-generate-a-warning cases look like this:

> Unused << NS_WARN_IF(NS_FAILED(rv));

People have already decided that this kind of use is exceptional enough that it
warrants the |Unused <<| to make it clear that it's deliberate. So let's make
that mandatory by adding MOZ_MUST_USE for NS_warn_if_impl(), which is the
function that underlies NS_WARN_IF.
This makes the use of |Unused <<| mandatory for NS_WARN_IF uses outside of
conditions, which increases consistency. It also avoids lots of false positives
for Coverity's CHECKED_RETURN warning.
Attachment #8786619 - Flags: review?(erahm)
This version fixes some compile errors that a try push found.
Attachment #8786689 - Flags: review?(erahm)
Attachment #8786619 - Attachment is obsolete: true
Attachment #8786619 - Flags: review?(erahm)
I like the idea of this but I think we'd be better served converting stuff like:

> Unused << NS_WARN_IF(*aOffset < 0);
> Unused << NS_WARN_IF(!mLast);

to |NS_WARN_IF_FALSE| (or maybe some new version). This way we'd avoid evaluating the code on non-debug builds (as-is the warning is debug only anyhow). Of course this means we'd have to negate those statements.

> Unused << NS_WARN_IF(aRv.Failed());
> Unused << NS_WARN_IF(NS_FAILED(rv));

are candidates as well, adding something like |NS_WARN_IF_FAILED(rv)| might make sense.

All that said, although it seems like this would be a good time to make the change, I'd be okay just filing a follow up.
This change avoids lots of false positives for Coverity's CHECKED_RETURN
warning, caused by NS_WARN_IF's current use in both statement-style and
expression-style.

In the case where the code within the NS_WARN_IF has side-effects, I made the
following change.

> NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));
> -->
> Unused << NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));

In the case where the code within the NS_WARN_IF lacks side-effects, I made the
following change.

> NS_WARN_IF(!condWithoutSideEffects);
> -->
> NS_WARNING_ASSERTION(condWithoutSideEffects, "msg");

This has two improvements.
- The condition is not evaluated in non-debug builds.
- The sense of the condition is inverted to the familiar "this condition should
  be true" sense used in assertions.

A common variation on the side-effect-free case is the following.

> nsresult rv = Fn();
> NS_WARN_IF_(NS_FAILED(rv));
> -->
> DebugOnly<nsresult rv> = Fn();
> NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Fn failed");
Attachment #8788118 - Flags: review?(erahm)
Attachment #8786689 - Attachment is obsolete: true
Attachment #8786689 - Flags: review?(erahm)
Comment on attachment 8788118 [details] [diff] [review]
Use MOZ_MUST_USE with NS_warn_if_impl()

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

Looks good. Thanks for taking the time to make all the NS_WARNING_ASSERTION changes, it's much clearer now.

::: dom/presentation/PresentationSessionInfo.h
@@ +139,5 @@
>      mReason = aReason;
>  
>      // Notify session state change.
>      if (mListener) {
> +      Unused <<

Any reason for the DebugOnly deviation here?

::: toolkit/components/terminator/nsTerminator.cpp
@@ +343,5 @@
>    }
>  
>    for (size_t i = 0; i < ArrayLength(sShutdownSteps); ++i) {
>      DebugOnly<nsresult> rv = os->AddObserver(this, sShutdownSteps[i].mTopic, false);
>  #if defined(DEBUG)

Can remove the #if

::: xpcom/glue/nsDebug.h
@@ +41,5 @@
> + * However, note that the argument to this macro is evaluated in all builds. If
> + * you just want a warning assertion, it is better to use NS_WARNING_ASSERTION
> + * (which evaluates the condition only in debug builds) like so:
> + *
> + *   NS_WARNING_ASSERT(NS_SUCCEEDED(rv), "operation failed");

nit: NS_WARNING_ASSERTION
Attachment #8788118 - Flags: review?(erahm) → review+
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/177f41cecedb
Use MOZ_MUST_USE with NS_warn_if_impl(). r=erahm.
jorgk: I don't think this will cause problems with comm-central -- I did a test build, and I don't see any NS_WARN_IF occurrences in comm-central that aren't within |if| conditions -- but I'll needinfo you here just in case, so you know where to look if something does go wrong.
Flags: needinfo?(jorgk)
Thanks for the heads-up, much appreciated!
Flags: needinfo?(jorgk)
https://hg.mozilla.org/mozilla-central/rev/177f41cecedb
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
The latest Coverity run just occurred, and it says:

> 250 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.

Yay! Most of these fixes will be due to this change. (10--20 is a more typical number of fixes for one of these reports.) This means the signal-to-noise ratio of the CHECKED_RETURN check should be a lot higher now.
Keywords: coverity
(In reply to Nicholas Nethercote [:njn] from comment #10)
> The latest Coverity run just occurred, and it says:
> 
> > 250 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> Yay! Most of these fixes will be due to this change. (10--20 is a more
> typical number of fixes for one of these reports.) This means the
> signal-to-noise ratio of the CHECKED_RETURN check should be a lot higher now.

I think most of those fixes were because of the triage for move assignment operator implementation - all of them false-positive, around 80 issues and 70 issues of variables null checked after dereferences in code generated by codegen.py
Nicholas this is a very very very good patch to silence the false-positive generated by this case. As a side note what I plan to do is to correlate our in code variables annotation, for example the skip of uninitialized member variables check, with an annotation from coverity. Because the bulk of our issues is from this kind of checkers. Uint ctor
You need to log in before you can comment on or make changes to this bug.