Fix sandbox::PolicyBase leak

Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/b6a4ff86c730756a73d63cc882ef818fb7818a53
Cr-Commit-Position: refs/heads/master@{#417313}

[jbauman, 2016-09-07 21:12:14]

Description was changed from

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr and RefCountedThreadSafe instead to fix that
and prevent it from happening in the future.
==========

to

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr and RefCountedThreadSafe instead to fix that
and prevent it from happening in the future.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[jbauman, 2016-09-07 21:12:21]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 21:13:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:32:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:32:50]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jbauman, 2016-09-07 22:35:19]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:36:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-09-07 22:39:43]

jbauman@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2016-09-07 22:44:52]

thanks for doing this.

Will review soon. as an aside, looks a lot like
https://codereview.chromium.org/1378523002/

[jbauman, 2016-09-07 23:07:12]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 23:08:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-09-07 23:13:52]

On 2016/09/07 22:44:52, Will Harris wrote:
> thanks for doing this.
> 
> Will review soon. as an aside, looks a lot like
> https://codereview.chromium.org/1378523002/

Thanks for pointing that out. Based on
https://codereview.chromium.org/1460903002#msg7 I've decided to switch back from
RefCountedThreadSafe to custom refcounting (though it'd most likely be safe in
this case). I just had to switch the starting refcount to 0 to be compatible
with scoped_refptr.

[jbauman, 2016-09-07 23:26:03]

Description was changed from

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr and RefCountedThreadSafe instead to fix that
and prevent it from happening in the future.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Will Harris, 2016-09-07 23:28:33]

I think cpu really disliked the refcount starting at 0.

https://codereview.chromium.org/1456663003#msg11

I'm not sure what the best option is here, I think cpu has stronger views on
this than I do. Either way I don't understand how the refcount can start at a
different value without changing the checks in the code (see comment)

https://codereview.chromium.org/2316333003/diff/40001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/2316333003/diff/40001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:164: if (0 ==
::InterlockedDecrement(&ref_count))
does this check not need to change now?

[jbauman, 2016-09-07 23:35:45]

On 2016/09/07 23:28:33, Will Harris wrote:
> I think cpu really disliked the refcount starting at 0.

That's how RefCountedThreadSafeBase and RefCountedBase work, so it'd make sense
to be consistent across the codebase. scoped_refptr doesn't work with it
otherwise unless there's a hacky Release() thrown in.
> 
> https://codereview.chromium.org/1456663003#msg11
> 
> I'm not sure what the best option is here, I think cpu has stronger views on
> this than I do. Either way I don't understand how the refcount can start at a
> different value without changing the checks in the code (see comment)
> 
>
https://codereview.chromium.org/2316333003/diff/40001/sandbox/win/src/sandbox...
> File sandbox/win/src/sandbox_policy_base.cc (right):
> 
>
https://codereview.chromium.org/2316333003/diff/40001/sandbox/win/src/sandbox...
> sandbox/win/src/sandbox_policy_base.cc:164: if (0 ==
> ::InterlockedDecrement(&ref_count))
> does this check not need to change now?

The scoped_refptr constructor always increments the refcount, so as soon as it's
placed in that the refcount will be 1. That's unlike before, where there was no
initial AddRef on creation.

[cpu_(ooo_6.6-7.5), 2016-09-08 00:18:50]

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

[cpu_(ooo_6.6-7.5), 2016-09-08 00:18:51]

Yeah this comes back every year. I feel strongly that refcounting should start
at 1. There is a long history of bugs related to this because the invariant of 
> 0 --> alive is not held during ctor.

Which I can expand if needed.

For the record chromium is the oddball here, blink and mojo start at 1.

[jbauman, 2016-09-08 00:33:18]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 00:33:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-09-08 00:35:06]

On 2016/09/08 00:18:51, cpu wrote:
> Yeah this comes back every year. I feel strongly that refcounting should start
> at 1. There is a long history of bugs related to this because the invariant of

> > 0 --> alive is not held during ctor.
> 
> Which I can expand if needed.
> 
> For the record chromium is the oddball here, blink and mojo start at 1.

Ok, fair enough. Switched it to start with refcount 1 and Release right after
it's put into the scoped_refptr.

[Will Harris, 2016-09-08 00:48:04]

lgtm

[jbauman, 2016-09-08 00:53:36]

Also FYI, I think the original Release() call was removed in
https://codereview.chromium.org/1415483008 .

[commit-bot: I haz the power, 2016-09-08 01:52:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 01:52:02]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-09-08 16:41:00]

Description was changed from

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[jbauman, 2016-09-08 16:41:06]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2016-09-08 16:41:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 16:45:57]

Description was changed from

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2016-09-08 16:45:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 16:48:03]

Description was changed from

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/b6a4ff86c730756a73d63cc882ef818fb7818a53
Cr-Commit-Position: refs/heads/master@{#417313}
==========

[commit-bot: I haz the power, 2016-09-08 16:48:07]

Patchset 4 (id:??) landed as
https://crrev.com/b6a4ff86c730756a73d63cc882ef818fb7818a53
Cr-Commit-Position: refs/heads/master@{#417313}

[pastarmovj, 2016-10-26 18:23:58]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2453053002/ by pastarmovj@chromium.org.

The reason for reverting is: The change breaks Chrome on RDP like WIndows
Terminal Services Published Apps or Citrix ZenApp..