Fix handle leak on broker error.

Fix handle leak on broker error.

Also avoid double Close of internal_->socket_for_renderer
on this error.

BUG= none
TEST= none

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

[Mark Seaborn, 2013-07-11 15:20:11]

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:547:
DuplicateHandle(nacl_host_message_filter_->PeerHandle(),
Why don't you use CloseHandle() to close the handle?

[halyavin, 2013-07-11 15:28:52]

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:547:
DuplicateHandle(nacl_host_message_filter_->PeerHandle(),
On 2013/07/11 15:20:11, Mark Seaborn wrote:
> Why don't you use CloseHandle() to close the handle?

CloseHandle can't close a handle belonging to a different process.
See documentation just before example:
http://msdn.microsoft.com/en-us/library/ms724251(v=vs.85).aspx#code-snippet-2

[Mark Seaborn, 2013-07-11 19:27:40]

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:547:
DuplicateHandle(nacl_host_message_filter_->PeerHandle(),
On 2013/07/11 15:28:52, halyavin wrote:
> On 2013/07/11 15:20:11, Mark Seaborn wrote:
> > Why don't you use CloseHandle() to close the handle?
> 
> CloseHandle can't close a handle belonging to a different process.

Oh right, of course. :-)  I didn't realise you were closing another process's
handle.  Maybe commenting "Close duplicated handle in remote process" would make
that more obvious.

Would a simpler fix be to move the BrokerAddTargetPeer() call to before the
DuplicateHandle() call, so that there's nothing to clean up if
BrokerAddTargetPeer() fails?


@Justin: What's the status of the Chrome IPC refactoring that would remove the
need to open-code DuplicateHandle() everywhere?

[halyavin, 2013-07-12 08:17:13]

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/24002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:547:
DuplicateHandle(nacl_host_message_filter_->PeerHandle(),
On 2013/07/11 19:27:40, Mark Seaborn wrote:
> Would a simpler fix be to move the BrokerAddTargetPeer() call to before the
> DuplicateHandle() call, so that there's nothing to clean up if
> BrokerAddTargetPeer() fails?
Thanks! Done.

[Mark Seaborn, 2013-07-12 15:18:03]

LGTM

https://codereview.chromium.org/18474005/diff/37001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/37001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:521: #endif
Add empty line after this?  This was a separate block before.

[commit-bot: I haz the power, 2013-07-12 16:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/18474005/56001

[halyavin, 2013-07-12 16:45:22]

https://codereview.chromium.org/18474005/diff/37001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18474005/diff/37001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:521: #endif
On 2013/07/12 15:18:03, Mark Seaborn wrote:
> Add empty line after this?  This was a separate block before.

Done.

[commit-bot: I haz the power, 2013-07-12 19:26:13]

Change committed as 211440