1237712 - Warn when the value of UniquePtr::release() is unused

Status: RESOLVED DUPLICATE of bug 1248851

(Core :: MFBT, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This warning would have caught the leak in bug 1237709. The other places that call release() without using the return don't leak but general use a very weird .get(); .release() idiom that can just be fixed.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Warn when the value of UniquePtr::release() is unused

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8705283 [details] [diff] [review]
Warn when the value of UniquePtr::release() is unused

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

Good catch.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> This warning would have caught the leak in bug 1237709. The other places
> that call release() without using the return don't leak but general use a
> very weird .get(); .release() idiom that can just be fixed.

Looking more closely at these, it looks like the idiom is actually:

rv = doSomethingThatMightFail(p.get()); // give up ownership if success
if (FAILED(rv))
    return;
p.release(); // note that we don't have ownership anymore.

This isn't great but is at least sort of sane...

Comment 4

[Nathan Froyd [:froydnj]]

Wouldn't that be better dealt with by Move()'ing |p| into the call to doSOmethingThatMightFail?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Wouldn't that be better dealt with by Move()'ing |p| into the call to
> doSOmethingThatMightFail?

It would be, but doSomethingThatMightFail takes a raw ptr. (I haven't looked into why this is the case in many of the places)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #4)
> > Wouldn't that be better dealt with by Move()'ing |p| into the call to
> > doSOmethingThatMightFail?
> 
> It would be, but doSomethingThatMightFail takes a raw ptr. (I haven't looked
> into why this is the case in many of the places)

Maybe we should try hard to fix that. :)

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> > It would be, but doSomethingThatMightFail takes a raw ptr. (I haven't looked
> > into why this is the case in many of the places)
> 
> Maybe we should try hard to fix that. :)

My working assumption would have been that they were XPCOM methods, but I guess that doesn't make sense unless we're talking [notxpcom] methods.  Which we probably aren't.

I'm pretty sure I've seen some of these for cases where a variable's ownership was passed into a thread, and by thread API nature the only thing that can be passed in is a void*.  I suppose those could be encapsulated a little so at least there are fewer such places to audit, or we could annotate each unconsumed release() with something indicating we've looked at it.

Incidentally, I'd be curious to know how you generated this list of places, seeing as 

https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-ref%3Amozilla%3A%3AUniquePtr%3A%3Arelease%28%29

doesn't work (presumably because templates).  Hand-rolled clang plugin?

Comment 8

[:Cykesiopka]

Looks like this was fixed as part of Bug 1248851:
https://hg.mozilla.org/mozilla-central/rev/3374795cf415