421998 - analysis wanted: functions must either return or dereference an already_AddRefed, only once

Status: RESOLVED DUPLICATE of bug 1180993

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

The already_AddRefed templated class is a lightweight wrapper around an interface pointer that indicates that the pointer has been addrefed. It is typically used when assigning to a nsCOMPtr.

The rule we should enforce is: for any already_AddRefed value, you must do one of the following things, and you may only do any of these once:

* copy it to another alreadyAddRefered
* assign it to a nsCOMPtr
* return it

There may be an additional case where you call .get() on it and set an outparam with that value, but let's ignore that case until we find it.

So the following code snippets should be flagged as errors:

already_AddRefed<nsIFoo> GetAFoo();
already_AddRefed<nsIFoo> DoSomething(int);

void GetTwice()
{
  already_AddRefed<nsIFoo> aafoo = GetAFoo();
  nsCOMPtr<nsIFoo> foo1 = aafoo;
  nsCOMPtr<nsIFoo> foo2 = aafoo; // bad, called .get() twice
}

void BadGet()
{
  GetAFoo().get()->CallMethod(); // bad, called .get without assigning
                                 // to a nsCOMPtr
}

void IgnoreValue()
{
  DoSomething(3); // bad, ignored already_AddRefed result
}

Comment 1

[jag (Peter Annema)]

Attachment: Runtime debug checks

Not as nice as static analysis, of course, but I've been running with this and haven't seen either assertion yet.

__BASE_FILE__ is a GCC thing, though MS might've picked it up or have something similar. If anyone thinks I should turn this into a real patch I'll look into that.

Comment 2

[Karl Tomlinson (:karlt)]

Could we treat the already_AddRefed similar to an auto_ptr?
i.e. null out the raw ptr when its value is taken,
     and Release on destruction if null?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is done dynamically in bug 967364 (with something like the first half of what karlt suggests in comment 2).