Actually, maybe someone who's more familiar with how the sandbox is supposed to work could try and fix this. I guess it'd need to monitor all the child processes created on a PolicyBase to wait for them to exit before destroying the PolicyBase.

ricky doens't have a windows machine right now but he had a very similar CL and knows the sandbox pretty well ( :) ) so might have some suggestion here. Otherwise, I can have a go landing something.

If I'm reading correctly, the issue is that in the case where we have a Job, the object ownership looks like this:

BrokerServices -> JobTracker -> Policy -> TargetProcess

In the case where we don't have a job, we currently leak the policy. The scoped_refptr change fixed that leak, so the policy gets destroyed, which destroys objects (like the IPC server) that are needed to support the process.

A long term fix for this would be to fix the ownership to make more sense. Here are some notes I wrote a while back on what I think it should look like:

Current state:
  JobTracker
    Policy
      TargetProcess
        Tokens
        IPC server

Desired state:
  JobTracker
    TargetProcess
      Tokens
      Policy (ref)
      IPC server

This will likely take some time to do (CCed forshaw@, who mentioned interest in this in the past).

In the short term, wfh@ made the sneaky suggestion of only leaking the reference in the no-job case - that should plug the memory leak for the vast majority of users who are using the sandbox with jobs. The change would basically be jbauman@'s change https://codereview.chromium.org/2316333003 with something like the following added at line 466 in sandbox/win/src/broker_services.cc:


  // The policy needs to outlive the process. Currently, we do not have
  // an appropriate owner for the policy when the target process is not
  // in a job, so we must leak the policy.
  // TODO(xxx): Make TargetProcess hold a ref to the policy instead of
  // the policy owning TargetProcess.
  policy_base.Release();

Like wfh@ mentioned, I don't have a windows machine to test with now - would someone mind patching this in and testing it out? To test this, I'd do the following:

1) Patch in jbauman@'s change
2) Run Chrome with --allow-no-sandbox-job, ensure that it crashes
3) Apply above suggested change
4) Run Chrome with --allow-no-sandbox-job, should not crash

thanks ricky, I think that approach sounds sensible, but I think I'd like to see a (failing) unit test for the jbauman change to make sure it's fixed. I can try and look at this tomorrow but after that I am OOO for a bit, so perhaps forshaw@ could dive in, especially if this is actually causing a memory regression.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d47e639710463d061bb385e60d8395161958284

commit 7d47e639710463d061bb385e60d8395161958284
Author: jbauman <jbauman@chromium.org>
Date: Mon Nov 28 23:41:31 2016

Add unittest that sandbox::PolicyBase outlives child process.

BUG=663019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2513163002
Cr-Commit-Position: refs/heads/master@{#434778}

[modify] https://crrev.com/7d47e639710463d061bb385e60d8395161958284/sandbox/win/src/policy_target_test.cc
[modify] https://crrev.com/7d47e639710463d061bb385e60d8395161958284/sandbox/win/tests/common/controller.cc
[modify] https://crrev.com/7d47e639710463d061bb385e60d8395161958284/sandbox/win/tests/common/controller.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14944d76e12f71f7d1a45832b303eafff0a708c7

commit 14944d76e12f71f7d1a45832b303eafff0a708c7
Author: jbauman <jbauman@chromium.org>
Date: Wed Dec 07 22:34:46 2016

Terminate child process if sandbox::TargetProcess is destroyed.

Leaking the ipc server doesn't work as it has raw pointers to other
classes (like the Dispatcher) that will be destroyed.

BUG=663019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2517703003
Cr-Commit-Position: refs/heads/master@{#437093}

[modify] https://crrev.com/14944d76e12f71f7d1a45832b303eafff0a708c7/sandbox/win/src/target_process.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d

commit 17e392b12a3b8a59fc3ca9265d1d89cda2a5509d
Author: jbauman <jbauman@chromium.org>
Date: Mon Feb 06 20:23:17 2017

Reland "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.

BUG=663019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2646043002
Cr-Commit-Position: refs/heads/master@{#448371}

[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/content/common/sandbox_init_win.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/content/common/sandbox_win.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/sandbox_poc/main_ui_window.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/src/broker_services.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/src/broker_services.h
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/src/policy_target_test.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/src/sandbox.h
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/tests/common/controller.cc
[modify] https://crrev.com/17e392b12a3b8a59fc3ca9265d1d89cda2a5509d/sandbox/win/tests/common/controller.h

Does this CL complete this task and is the status now fixed?

I think it still leaks if --allow-no-sandbox-job is specified on the command line .

I think we could leave it as this since this is a very edge case used by a ever diminishing (but non-zero) population of users running Chrome on Windows Server 2008 R2 in a terminal services environment. Some day in the not so distant future we will drop support for Win 7 which will include 2k8 R2 and then we can completely remove the code servicing this flag too.