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. 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(); 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. 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. - Seth > On Jun 30, 2015, at 12:02 PM, Nathan Froyd <nfr...@mozilla.com> wrote: > > Bug 1161627 has landed on inbound, which converts all uses of > mozilla::TemporaryRef to already_AddRefed and removes TemporaryRef. > (already_AddRefed moved to MFBT several months ago, in case you were > wondering about the spreading of XPCOM concepts.) TemporaryRef was added > for easier porting of code from other engines, but as it has not been used > much for that purpose and it led to somewhat less efficient code in places, > it was deemed a failed experiment. > > -Nathan > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
