mojo: Don't leak handles when assigning to ScopedHandles.

mojo: Don't leak handles when assigning to ScopedHandles.

BUG=374520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278015

[viettrungluu, 2014-06-17 22:58:25]

https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core.h
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core....
mojo/public/cpp/system/core.h:110: if (handle_.is_valid() && handle_.value() !=
other.object->get().value())
Since a handle should only be in a single ScopedHandleBase<> at any particular
time, you could simplify this to:

if (other.object != this)
  CloseIfNecessary();

(CloseIfNecessary() checks for validity).

Or maybe better (more self-evidently correct):

if (other.object != this) {
  CloseIfNecessary();
  handle_ = other.object->release();
}
return *this;

You might also add an |assert(handle_.value() != other.object->get().value());|
before the |CloseIfNecessary();| (inside the if block).

[Elliot Glaysher, 2014-06-17 23:08:28]

https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core.h
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core....
mojo/public/cpp/system/core.h:110: if (handle_.is_valid() && handle_.value() !=
other.object->get().value())
On 2014/06/17 22:58:25, viettrungluu wrote:
> Since a handle should only be in a single ScopedHandleBase<> at any particular
> time, you could simplify this to:
> 
> if (other.object != this)
>   CloseIfNecessary();
> 
> (CloseIfNecessary() checks for validity).
> 
> Or maybe better (more self-evidently correct):
> 
> if (other.object != this) {
>   CloseIfNecessary();
>   handle_ = other.object->release();
> }
> return *this;
> 
> You might also add an |assert(handle_.value() !=
other.object->get().value());|
> before the |CloseIfNecessary();| (inside the if block).

Switched to if (other.object != this). Didn't add the assert, because wouldn't
that trigger on an assignment of a closed handle to a closed handle?

[viettrungluu, 2014-06-17 23:34:42]

On 2014/06/17 23:08:28, Elliot Glaysher wrote:
> https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core.h
> File mojo/public/cpp/system/core.h (right):
> 
>
https://codereview.chromium.org/336313003/diff/1/mojo/public/cpp/system/core....
> mojo/public/cpp/system/core.h:110: if (handle_.is_valid() && handle_.value()
!=
> other.object->get().value())
> On 2014/06/17 22:58:25, viettrungluu wrote:
> > Since a handle should only be in a single ScopedHandleBase<> at any
particular
> > time, you could simplify this to:
> > 
> > if (other.object != this)
> >   CloseIfNecessary();
> > 
> > (CloseIfNecessary() checks for validity).
> > 
> > Or maybe better (more self-evidently correct):
> > 
> > if (other.object != this) {
> >   CloseIfNecessary();
> >   handle_ = other.object->release();
> > }
> > return *this;
> > 
> > You might also add an |assert(handle_.value() !=
> other.object->get().value());|
> > before the |CloseIfNecessary();| (inside the if block).
> 
> Switched to if (other.object != this). Didn't add the assert, because wouldn't
> that trigger on an assignment of a closed handle to a closed handle?

Uh, good point. (If we really wanted an assertion, we'd have to exclude that
case, I guess.)

LGTM.

[Elliot Glaysher, 2014-06-17 23:39:16]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-17 23:43:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/336313003/20001

[commit-bot: I haz the power, 2014-06-18 10:23:24]

Change committed as 278015