Fix new[]/delete mismatches in sandbox handle code by switching to scoped_ptr<C, base::FreeDeleter>.

Fix new[]/delete mismatches in sandbox handle code by switching to scoped_ptr<C, base::FreeDeleter>.

BUG=246802
R=jschuh@chromium.org
TEST=browser_tests under Dr. Memory

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

[rvargas (doing something else), 2013-06-05 22:27:36]

drive by

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer.cc
File sandbox/win/src/handle_closer.cc (right):

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer...
sandbox/win/src/handle_closer.cc:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
We should not update the year on CR headers.

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer...
sandbox/win/src/handle_closer.cc:188: CHECK(name.get());
do we really net a check here?

[Derek Bruening, 2013-06-06 04:33:25]

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer.cc
File sandbox/win/src/handle_closer.cc (right):

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer...
sandbox/win/src/handle_closer.cc:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2013/06/05 22:27:36, rvargas wrote:
> We should not update the year on CR headers.

Hmm, yes, this is in http://www.chromium.org/developers/coding-style.  Time to
disable emacs copyright-update for Cr editing.

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer...
sandbox/win/src/handle_closer.cc:188: CHECK(name.get());
On 2013/06/05 22:27:36, rvargas wrote:
> do we really net a check here?

Is there a Cr convention on OOM handling?  I saw many calls to malloc that do
not check the return value, and some that do.  I'm guessing the ones that don't
are assuming the shim wrapper is in place.  But for component build, the shim is
disabled and malloc goes straight to msvcrt.  Thus for component build OOM will
not call the new handler and will just return NULL.  If QueryObject were a
direct system call, then yes it would just fail nicely on a NULL: but as a
library layer it might go and crash strangely on us.  Hence, an explicit check
on the return value of malloc seems reasonable, right?

[jschuh, 2013-06-06 13:21:20]

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer.cc
File sandbox/win/src/handle_closer.cc (right):

https://codereview.chromium.org/16404003/diff/1/sandbox/win/src/handle_closer...
sandbox/win/src/handle_closer.cc:188: CHECK(name.get());
On 2013/06/06 04:33:25, Derek Bruening wrote:
> On 2013/06/05 22:27:36, rvargas wrote:
> > do we really net a check here?
> 
> Is there a Cr convention on OOM handling?  I saw many calls to malloc that do
> not check the return value, and some that do.  I'm guessing the ones that
don't
> are assuming the shim wrapper is in place.  But for component build, the shim
is
> disabled and malloc goes straight to msvcrt.  Thus for component build OOM
will
> not call the new handler and will just return NULL.  If QueryObject were a
> direct system call, then yes it would just fail nicely on a NULL: but as a
> library layer it might go and crash strangely on us.  Hence, an explicit check
> on the return value of malloc seems reasonable, right?

Wouldn't a DCHECK be fine then? Since for all intents and purposes the component
build is debug only, and the CHECK is superfluous in release builds.

[Derek Bruening, 2013-06-06 15:02:47]

On 2013/06/06 13:21:20, Justin Schuh wrote:
> Wouldn't a DCHECK be fine then? Since for all intents and purposes the
component
> build is debug only, and the CHECK is superfluous in release builds.

Sure, sounds fine.

New patch uploaded: PTAL.

[jschuh, 2013-06-08 00:42:34]

lgtm

[commit-bot: I haz the power, 2013-06-08 00:43:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruening@chromium.org/16404003/6001

[commit-bot: I haz the power, 2013-06-08 06:15:48]

Change committed as 205053