On Tue, Jul 7, 2015 at 9:59 AM, Seth Fowler <mfow...@mozilla.com> wrote:
> I brought this up on IRC, but I think this is worth taking to the list: > > Replacing TemporaryRef with already_AddRefed has already caused at least > one leak because already_AddRefed does not call Release() on the pointer it > holds if nothing takes ownership of that pointer before its destructor > runs. TemporaryRef did do that, and this seems like a strictly safer > approach. > > My question is: why doesn’t already_AddRefed call Release() in its > destructor? The fact that it doesn’t seems to me to be a profoundly > unintuitive footgun. > We have a version of already_AddRefed that automatically releases the value it holds when it's destroyed; it's called nsCOMPtr. I don’t think we can trust reviewers to catch this, either. The fact that > Foo() is declared as: > > already_AddRefed<Bar> Foo(); > > won’t necessarily be obvious from a call site, where the reviewer will > just see: > > Foo(); > Foo should be MOZ_WARN_UNUSED_RESULT (or whatever that annotation is) then, as should anything that returns an already_AddRefed. That call will leak, and if we don’t happen to hit that code path in our > tests, we may not find out about it until after shipping it. > Ensuring that code that is untested works is in general a hard thing to do. I'd prefer that people wrote tests. (yes, there may be edge cases in which this isn't possible) I’d prefer that already_AddRefed just call Release(), but if there are > performance arguments against that, then we should be checking for this > pattern with static analysis. I don’t think the situation as it stands > should be allowed to remain for long. There is currently a dynamic analysis (in the form of a fatal assertion) in the already_AddRefed dtor that I added last year. - Kyle _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
