Sandbox: Remove ::CloseHandle from BrokerServices.

Sandbox: Remove ::CloseHandle from BrokerServices.

This CL upgrades local members and support structs to store ScopedHandles.

BUG=426577
Committed: https://crrev.com/3a4914edd558bcf5b47d11d64aa3cba3127c6e4e
Cr-Commit-Position: refs/heads/master@{#340129}

[rvargas (doing something else), 2015-07-14 01:05:10]

rvargas@chromium.org changed reviewers:
+ cpu@chromium.org

[rvargas (doing something else), 2015-07-16 00:53:40]

On 2015/07/14 01:05:10, rvargas wrote:

ping

[cpu_(ooo_6.6-7.5), 2015-07-17 19:33:24]

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:59: JobTracker(base::win::ScopedHandle cjob,
sandbox::PolicyBase* cpolicy)
I don't think you need to add 'c' to the args. I believe c++ does not get
confused here.

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:80: HANDLE old_job_handle = job.Get();
so old_job_handle is more really invalid_job_handle ?

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:153: // TODO(rvargas): Store a pair of
ScopedHandles when that becomes possible.
the 153 comment, which 'pair' are you referring to?

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:154: struct BrokerServicesBase::Tokens {
I feel like maybe this class should be named TokenPair, but not too attached to
that.

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:207: WAIT_TIMEOUT ==
::WaitForSingleObject(job_thread_.Get(), 1000)) {
but if job thread is not valid WFSO will return an error ... so the IsValid() is
not needed?

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:213: STLDeleteElements(&tracker_list_);
nice!

[rvargas (doing something else), 2015-07-20 20:06:30]

Thanks, ptal.

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:59: JobTracker(base::win::ScopedHandle cjob,
sandbox::PolicyBase* cpolicy)
On 2015/07/17 19:33:24, cpu wrote:
> I don't think you need to add 'c' to the args. I believe c++ does not get
> confused here.

I'm not adding 'c'... I'm just moving this code a little to have methods first
and members last. But sure, removed the 'c'. (even if c++ doesn't get confused
I'm not sure the result is more readable though... but I don't like cfoo
either).

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:80: HANDLE old_job_handle = job.Get();
On 2015/07/17 19:33:24, cpu wrote:
> so old_job_handle is more really invalid_job_handle ?

yes, it becomes stale next line. (saying this was the old_handle doesn't really
tells me that the handle is valid :p ). Renamed to stale_ (but it is assigned a
handle that is not stale_... oh well).

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:153: // TODO(rvargas): Store a pair of
ScopedHandles when that becomes possible.
On 2015/07/17 19:33:24, cpu wrote:
> the 153 comment, which 'pair' are you referring to?

Reworded

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:154: struct BrokerServicesBase::Tokens {
On 2015/07/17 19:33:24, cpu wrote:
> I feel like maybe this class should be named TokenPair, but not too attached
to
> that.

Done.

https://codereview.chromium.org/1228373003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:207: WAIT_TIMEOUT ==
::WaitForSingleObject(job_thread_.Get(), 1000)) {
On 2015/07/17 19:33:24, cpu wrote:
> but if job thread is not valid WFSO will return an error ... so the IsValid()
is
> not needed?

I don't think it's good form to pass invalid handles to APIs.

[cpu_(ooo_6.6-7.5), 2015-07-23 17:56:27]

lgtm

sorry for the delay.

[rvargas (doing something else), 2015-07-23 18:11:51]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-07-23 18:13:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228373003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1228373003/20001

[commit-bot: I haz the power, 2015-07-23 19:05:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-23 19:05:35]

Patchset 2 (id:??) landed as
https://crrev.com/3a4914edd558bcf5b47d11d64aa3cba3127c6e4e
Cr-Commit-Position: refs/heads/master@{#340129}