1292440 - Use MOZ_MUST_USE in NS_IMETHOD

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

One way to greatly extend the use of MOZ_MUST_USE is to use it with NS_IMETHODIMP
and friends. Doing this leads to hundreds of compile errors. Some of these are
true positives (the call really should be checked) and some are false positives
(the call doesn't need a check). Distinguishing these cases can be difficult.

So I propose to use a variant of mozilla::Unused (called mozilla::XUnused) at
all of these call sites. That gives us the following.

- It fixes the compile errors.

- It gives us protection against missing checks when writing new code.

- It will let us gradually audit all the uses of mozilla::XUnused to see if
  they need checks.

I also have a way of avoiding mozilla::XUnused for calls to infallible
functions.

I've already done big chunks of this and it's clear that many of the compile
errors are true positives. And bug 1292388 came out of this. So I think it's
worth doing.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use MOZ_MUST_USE in NS_IMETHODIMP and friends

This is a first pass. Things of note:

- I've only done NS_IMETHODIMP so far. NS_IMETHOD and NS_METHOD are still to
  come. Not sure yet whether to do them in this bug or a follow-up bug.

- I've only got it compiling on Linux (debug builds) so far.

- Start by reading the big comments in xpcom/base/nscore.h and mfbt/unused.h.
  That indicates what's going on. Other than those comments, basically all the 
  patch does is (a) add ~580 uses of |XUnused| and (b) convert ~165 occurrences
  of NS_IMETHODIMP to NS_INFALLIBLE_IMETHODIMP.

froydnj, I'm looking for feedback rather than a proper review for now. Does it
seem reasonable? It will make no-check-needed call sites more verbose, but that
makes the no-check-needed-ness clearer (explicit is better than implicit).
Also, it's unavoidable if we want to make it impossible to forget checks, which 
I'm pretty sure we do.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8778101 [details] [diff] [review]
Use MOZ_MUST_USE in NS_IMETHODIMP and friends

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

f+, I guess.  A couple comments:

* I understand the rationale for XUnused; you want to try and distinguish "we added these because we are currently too lazy to figure out nsresult checking" from ordinary (valid) uses of Unused.  How confident are we that XUnused is going to continue to be used in such a way?  Can we put XUnused in XPCOM, rather than MFBT?  Can we name it something different?  nsresultUnchecked?

* NS_INFALLIBLE_IMETHODIMP strikes me as a little weird; no checks for infallibility, especially when compared with [infallible] on attributes in IDL files, which does generate checks.  Food-for-though proposal:

- add [infallible_method] as an IDL annotation;
- have xpidl.py generate:

void MyMethodName(...Args...) {
  MOZ_ALWAYS_SUCCEEDS(MyMethodNameInfallible(...Args...));
}

- clients then implement MyMethodNameInfallible, and still use NS_IMETHODIMP.

Is that too clunky?

I can't think of other things to say, possibly because of the lateness of the hour.  I'll think about it overnight.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Driveby proposal: mark infallible methods with an attribute, [[infallible]] or something similar, and add a static analysis pass that verifies that such methods only return either *literal* NS_OK, or the result of another method marked [[infallible]].

Comment 4

[Nicholas Nethercote [inactive]]

> * I understand the rationale for XUnused; you want to try and distinguish
> "we added these because we are currently too lazy to figure out nsresult
> checking" from ordinary (valid) uses of Unused.  How confident are we that
> XUnused is going to continue to be used in such a way?  Can we put XUnused
> in XPCOM, rather than MFBT?  Can we name it something different? 
> nsresultUnchecked?

It's always possible that people could misuse it; that's unavoidable. I want to avoid a long name because that causes more line breaking and a good fraction of the XUnused occurrences will become Unused. Putting it in XPCOM is doable but that would also require changing #include directives when changing XUnused to Unused.

> * NS_INFALLIBLE_IMETHODIMP strikes me as a little weird; no checks for
> infallibility, especially when compared with [infallible] on attributes in
> IDL files, which does generate checks.  Food-for-though proposal:
> 
> - add [infallible_method] as an IDL annotation;
> - have xpidl.py generate:
> 
> void MyMethodName(...Args...) {
>   MOZ_ALWAYS_SUCCEEDS(MyMethodNameInfallible(...Args...));
> }
> 
> - clients then implement MyMethodNameInfallible, and still use NS_IMETHODIMP.
> 
> Is that too clunky?

Sounds doable, although I think you have the "Infallible" suffix in the wrong place -- xpidl.py would generate |void MyMethodNameInfallible| and the client would provide |NS_IMETHODIMP MyMethodName|, so you can call MyMethodName from script?

BTW, I now realize that a lot of the places in the patch where I used NS_INFALLIBLE_METHODIMP are for attribute getters/setters. I could probably add [infallible] to many of these and then change the call sites to use the infallible version instead. (Though now I see that [infallible] has restriction such as only working on builtin types. Hmm.)

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > * NS_INFALLIBLE_IMETHODIMP strikes me as a little weird; no checks for
> > infallibility, especially when compared with [infallible] on attributes in
> > IDL files, which does generate checks.  Food-for-though proposal:
> > 
> > - add [infallible_method] as an IDL annotation;
> > - have xpidl.py generate:
> > 
> > void MyMethodName(...Args...) {
> >   MOZ_ALWAYS_SUCCEEDS(MyMethodNameInfallible(...Args...));
> > }
> > 
> > - clients then implement MyMethodNameInfallible, and still use NS_IMETHODIMP.
> > 
> > Is that too clunky?
> 
> Sounds doable, although I think you have the "Infallible" suffix in the
> wrong place -- xpidl.py would generate |void MyMethodNameInfallible| and the
> client would provide |NS_IMETHODIMP MyMethodName|, so you can call
> MyMethodName from script?

The naming of the C++ virtual method and the naming of the JS method are orthogonal; see e.g. the [binaryname] attribute for IDL, which enables you to call the C++ thing whatever you want.

> BTW, I now realize that a lot of the places in the patch where I used
> NS_INFALLIBLE_METHODIMP are for attribute getters/setters. I could probably
> add [infallible] to many of these and then change the call sites to use the
> infallible version instead. (Though now I see that [infallible] has
> restriction such as only working on builtin types. Hmm.)

Lifting the [infallible] restriction on builtin types would be worthwhile!  I'm not completely sure how to do it with string return values, but I think it could totally be done for interface types.

Comment 6

[Nicholas Nethercote [inactive]]

> * NS_INFALLIBLE_IMETHODIMP strikes me as a little weird; no checks for
> infallibility, especially when compared with [infallible] on attributes in
> IDL files, which does generate checks.  Food-for-though proposal:
> 
> - add [infallible_method] as an IDL annotation;
> - have xpidl.py generate:
> 
> void MyMethodName(...Args...) {
>   MOZ_ALWAYS_SUCCEEDS(MyMethodNameInfallible(...Args...));
> }
> 
> - clients then implement MyMethodNameInfallible, and still use NS_IMETHODIMP.

Another possibility is to have another alternative to mozilla::Unused that takes an nsresult and asserts that it is NS_OK. So instead of this:

> MyMethodNameInfallible(a, b, c);    // asserts success inside

you'd have this:

> Infallible << MyMethodName(a, b, c);  // asserts inside operator<<

There might be a better name for this than |Infallible|. |AssertOk|, perhaps? You'd still use |Unused| for the "I know this might fail but it doesn't matter" case, though.

Comment 7

[Nicholas Nethercote [inactive]]

Changing the title: NS_IMETHOD is a better place for this that NS_IMETHODIMP, because it gives more coverage. NS_METHOD doesn't get used much, and might even be removable, so it's far less interesting.

Comment 8

[Nicholas Nethercote [inactive]]

After much experimenting, I've concluded that this approach is too blunt. There are too many IDL-declared operations where checking the result is unnecessary. A more fine-grained, incremental approach will be needed: bug 1295825.