179275 - should/can we stop functions that return an |AddRef|ed pointer as a function result from leaking in a void context?

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, enhancement)

Description

[Scott Collins]

See bug #172030 comment #47 for the detailed description, I will abstract here.
 For a function |f()| that returns a pointer result that has already been
|AddRef|ed, can we prevent |f()| from leaking when used in a |void| context? 
Additionally, our solution can be restricted to cases where |f()| has been
declared to return |already_AddRefed<T>|, and perhaps further restricted to
debug builds, if necessary.  And if we _can_ do this, _should_ we do this?  As
per bug #172030, I think it's certainly worth discussing.

My first question, though: is this really a problem that needs fixing?  Because
the typical function that returns such a pointer exists soley to return that
pointer, it seems unlikely that it would appear in a |void| context.  That is,
for example, one is unlikely to write

  CreateAFoo();

instead of

  nsCOMPtr<nsIFoo> fp = CreateAFoo();

There are special cases of this, in particular |do_QueryInterface|,
|do_QueryReferent|, |do_CreateInstance|, |do_GetService|, etc., etc.; however,
all these |do_...| routines are specially constructed already to be no-ops in a
void context.  Did I go too far in those cases?  I don't know ... maybe.  In
favor is that those routines really are no-ops in a void context.  It's not that
they don't leak, they don't do _anything_.  Solutions under consideration for
this bug don't turn the function calls into no-ops, they just stop them from
leaking, or else report the |void| use as an error.  Hmmm, maybe making all the
|do...| routines cause syntax errors in a |void| context would have been better.
 Worth considering?  Separate bug?  What's good for the goose should be good for
the gander.  If not-leaking, no-op-ing, or causing-a-syntax-error is good for
one category of this machinery, surely a parallel or identical solution is good
for the other.

Comment 1

[John Keiser (jkeiser)]

If all functions that returned already_AddRefed<T> were getters then this would
not be necessary.  In those cases the return value is absolutely implicit in the
function name.  But there are other cases that have side effects and return a
value, where callers might just care about the side effects.  For example,
consider a hypothetical function:

// Kick off load of a document into the current window
already_AddRefed<nsIRequest> loadDocument(nsAString& aURI);

The nsIRequest is a network channel that one can use to monitor the progress of
the document loading.  Some callers may want to load the document and then
monitor it:

nsCOMPtr<nsIRequest> request = loadDocument("http://www.google.com");
// Add yourself as an observer of the request

Others may not care about progress--they just want the document to load.

loadDocument("http://www.google.com");

Now they could create a throwaway request, and in the current model that is what
they *should* do.  But it would be natural for a programmer to forget about that
return value because the side effects do everything he wants.

Now, perhaps the function can be rewritten to use the COM calling convention,
but once people learn of already_AddRefed<T> they are *going to use it*.  It is
a much cleaner way to manipulate COM objects from classes with non-COM interfaces.

After all that, if we just assert in debug builds it won't cause one byte of
bloat for our shipped builds and it *will* provide a valuable service to
programmers when they screw this up.

Comment 2

[John Keiser (jkeiser)]

If we do bug #172030 comment #47 or something similar we will get assertions for
free on the do_XXX().  If you use do_QueryInterface(foo()) in void context the
already_AddRefed will not get nulled out and thus will assert when it is destructed.

Comment 3

[John Keiser (jkeiser)]

Regarding the possibility of protecting callers by releasing the value, we can
do that with a similar mechanism, nulling out the pointer on use and releasing
it on destruction if it is still there.  That raises some questions though:

(a) does it break the other COMPtr library rules?  I think so.  It makes
ownership non-explicit.  While that is not much of a problem in void context,
and it appears this will only *happen* in void context, it breaks the model.

(b) is it worth the few bits to fix this in all contexts or would an assertion
in debug builds be better to make optimized builds stay optimized?

I prefer the assertion in debug builds but you've got a good handle on your
model so I defer to your judgement.

Comment 4

[Scott Collins]

Do we have some concrete cases of these functions where their result might be in
jeopardy of leaking out through a |void| context?  jkeiser: are there any
functions that exist now that you are thinking of?  Or, if there are not
getter-only-as-a-side-effect functions returning |already_AddRefed|, then how
about the same, but returning raw |AddRef|ed pointers?

Comment 5

[John Keiser (jkeiser)]

Not only those that return T*, but those that do the COM T** method and are due
for restructuring since they are not in COM interfaces and do not return
anything other than NS_OK.

I have been studying these, and each of the major side effect functions
*generally* has to return nsresult because the side effect fails.  I think
returning nsresult is going to be the case for many such functions.  I am pretty
sure that non-nsresult-returning functions of this nature exist (I remember
having seen such things or saying "we can convert this"), but I am willing to
wait until I find them to do anything on this bug.

However, given that it *does* give us added protection, and does *not* add bloat
or peformance problems in optimized builds, I don't think we're talking about
anything onerous here.  And it *could* help catch errors early.  One of the
major things people get from nsCOMPtr is near-total safety (and early warning
when they do unsafe things), and we should continue that track record.

Comment 6

[Benjamin Smedberg]

I don't think there are many of these left, and if there are we should just be using warn-unused-result on them.