Unify PolicyBase into TargetPolicy.

Unify PolicyBase into TargetPolicy.

BUG=549319

[rickyz (no longer on Chrome), 2015-11-18 22:43:12]

rickyz@chromium.org changed reviewers:
+ cpu@chromium.org, wfh@chromium.org

[rickyz (no longer on Chrome), 2015-11-18 22:43:13]

Here's that PolicyBase/TargetPolicy unification I was talking about

[rickyz (no longer on Chrome), 2015-11-18 22:49:39]

On 2015/11/18 at 22:43:13, rickyz wrote:
> Here's that PolicyBase/TargetPolicy unification I was talking about
Forgot to mention, this depends on https://codereview.chromium.org/1456663003
(and the trybots don't seem very happy with this).

Also, I've left the file sandbox_policy_base named as is to keep git log
continuity, but happy to rename that if preferred. I also ran clang-format,
which is why there are a couple of formatting fixes in sandbox_policy_base.cc.

[rickyz (no longer on Chrome), 2015-12-15 01:12:42]

All right, rebased this now that the CL its depency is in, ptal.

This is just another cleanup to get rid of the two policy classes and the
casting we do between them.

[rickyz (no longer on Chrome), 2016-01-04 23:43:18]

rickyz@chromium.org changed reviewers:
+ jschuh@chromium.org

[rickyz (no longer on Chrome), 2016-01-04 23:43:18]

Adding another pair of eyes.

[cpu_(ooo_6.6-7.5), 2016-01-06 18:09:40]

https://codereview.chromium.org/1460903002/diff/60001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1460903002/diff/60001/content/common/sandbox_...
content/common/sandbox_win.cc:806: ? "Process.Sandbox.Lowbox.Launch.Error"
can you please remove the UMA_HISTO call?  I don't think anybody cares or
monitors these.

https://codereview.chromium.org/1460903002/diff/60001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/1460903002/diff/60001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy.h:40: 
Advance apologies for the rambling nature of the comment that follows.

the code here needs to be a "pure" interface class. That is, nothing more than a
vtable.

The reason is that unlike the other platforms, the sandbox is implemented in
chrome.exe but the broker interface is used from chrome.dll

So between two dlls you should not really transfer a 'fat' object pointer. There
are a variety of issues there. I can go into some details if you want but in the
windows universe this is one of the issues that COM solves, in C++ with pure
interfaces.

In practice this rule is violated a thousand times in the component build, for
example. However 1) we don't ship the component build to users and 2) the
component build uses the dynamic crt (the crt as a dll).

That last point is important because c++ memory management uses the module's
heap so when a fat object is used and there is method inlining, you could new an
object from one heap and free it in another.  Having the crt as a separate
module shared by all other modules solves that problem.

The above paragraph was true for the last 20 years but very recently we learned
that the microsoft crt no longer creates its own heap but uses the 'process
heap' thereby avoiding the problem. That however is an internal detail which
could change.

Although if that changes we could probably "fix it" because we hook the crt
memory functions, however we have a feature request pending to Microsoft which
will let us stop hooking the crt, which we should not do, except we have a very
powerful reason. wfh@ can go into the details.

Heaps is not the only problem, for example crt state such as the one in srttok
is also problematic.

Could we get away with this change? probably, after all, the component build
does sort of, kind of, work. It is certainly less correct.

So the long story short, unless there is a very powerful reason, we should keep
the broker and target interface pure virtual.

One last point worth mentioning is that because how we do upgrades in place of
chrome and where the chrome.exe and chrome.dll live we have a non zero set of
users which sometimes run the exe of an old version with the dll of a new
version. We are not fully sure how that happens. Maybe they tamper with the
installation.

If you want we can do a VC meeting with justin and will to go over the finer
points.

[Will Harris, 2016-05-03 21:34:51]

Ricky - I don't think this CL is a necessary prerequisite for the zygote work,
and for the reasons cpu mentioned I wonder whether this could be closed?