Make dispatcher a member of PolicyBase.

Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
Committed: https://crrev.com/60f931dc587fee8afbc933ec2d2fac8b24506eed
Cr-Commit-Position: refs/heads/master@{#364904}

[rickyz (no longer on Chrome), 2015-11-18 01:20:56]

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

[rickyz (no longer on Chrome), 2015-11-18 01:20:56]

Hey cpu@, jschuh@ and wfh@ mentioned that you were familiar with the history
behind the class hierarchy here: PolicyBase : Dispatcher, TargetPolicy. I would
like to unify PolicyBase and TargetPolicy into one class, and have the
dispatcher be a member - are you familiar with the reasons for the original
layout, and if there are any reasons against changing it? Thanks!

[cpu_(ooo_6.6-7.5), 2015-11-18 23:49:34]

Let's start the other way, can you expand on the reasons for changing it? 

What do you need to do that can't be done today?

[cpu_(ooo_6.6-7.5), 2015-11-18 23:57:10]

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

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:43: 
I don't think we use that nested style here, in other words I think the sandbox
files are:

namespace {
}

namespace sandbox {
}

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy_base.h (right):

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.h:158: scoped_ptr<Dispatcher> dispatcher_;
seems we are missing the include for scoped_ptr ?

[rickyz (no longer on Chrome), 2015-11-18 23:59:48]

On 2015/11/18 at 23:49:34, cpu wrote:
> Let's start the other way, can you expand on the reasons for changing it? 
> 
> What do you need to do that can't be done today?

This probably just falls under the category of cleanup - the whole thing started
when I wanted to use scoped_refptr instead of manual refcounting on
TargetPolicy, but according to folks on chromium-dev, it's recommended to
subclass from BaseRefCounted when using scoped_refptr. The
subclassing/refcounting doesn't really makes sense when PolicyBase multiple
inherits from Dispatcher and TargetPolicy, since you cannot do refcounting on a
Dispatcher* at all. Because of this, I thought it was cleaner to just combine
the two classes (and get rid of the weird casting we do between them as an extra
benefit).

[cpu_(ooo_6.6-7.5), 2015-11-19 00:18:04]

So we are mixing a few things

1- the cleanup as is going from multiple inheritance to composition is in
general fine with me.

2- chrome's base ref-counting class is fucked up. The count when the constructor
finishes is zero. Do not want. That said I don't remember what the sandbox does
so it could be as bad.

3. The code that executes in the sandboxed process in general cannot depend on
many base constructs because it can run very early in the process start, before
main() for example and the /base owners (me included) have not designed the code
for that case so one has to think hard about allowing some nice classes into the
sandbox circle-of-trust.

[cpu_(ooo_6.6-7.5), 2015-11-19 00:18:26]

also what weird cast are you talking about?

[rickyz (no longer on Chrome), 2015-11-19 00:36:06]

On 2015/11/19 at 00:18:04, cpu wrote:

> 2- chrome's base ref-counting class is fucked up. The count when the
constructor finishes is zero. Do not want. That said I don't remember what the
sandbox does so it could be as bad.
The current code does initialize the refcount to 1. I don't see what's so bad
about BaseRefCounted though - it's opinionated in the sense that you always want
to be dealing with a scoped_refptr, which will do the initial AddRef when you
construct it. It definitely beats manual refcounting when it comes to the
likelihood of making mistakes though.

> 3. The code that executes in the sandboxed process in general cannot depend on
many base constructs because it can run very early in the process start, before
main() for example and the /base owners (me included) have not designed the code
for that case so one has to think hard about allowing some nice classes into the
sandbox circle-of-trust.
Ah yeah, it's a similar story for the Linux sandbox - to the point that we've
added things like base/strings/safe_sprintf.h and documented them to be safe in
those special contexts. For what it's worth, I think the Policy classes are
currently only used in the parent process, where there shouldn't be much danger
(though the zygote changes I'm currently looking at will definitely introduce a
situation where we cannot use any APIs that might start threads).

The casts I was talking about are these two:

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san...
https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/br...

[rickyz (no longer on Chrome), 2015-11-19 00:38:20]

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

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:43: 
On 2015/11/18 at 23:57:10, cpu wrote:
> I don't think we use that nested style here, in other words I think the
sandbox files are:
> 
> namespace {
> }
> 
> namespace sandbox {
> }
Is there any reason behind that preference other than consistency? That nesting
would require me to prefix a bunch of things with sandbox:: in
TopLevelDispatcher, although maybe that's a signal that this should be in its
own cc/h file, which I'm happy to do if that's preferred.

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy_base.h (right):

https://codereview.chromium.org/1456663003/diff/20001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.h:158: scoped_ptr<Dispatcher> dispatcher_;
On 2015/11/18 at 23:57:10, cpu wrote:
> seems we are missing the include for scoped_ptr ?
Oops, fixed.

[cpu_(ooo_6.6-7.5), 2015-11-20 22:28:51]

namespace => yeah consistency. Another cc file is fine.

[cpu_(ooo_6.6-7.5), 2015-11-20 22:36:31]

And so the refcount starts at 1 so no, we can't use base's code. The issues with
ending ctor at refcount 0 are too long to explain but in general it is incorrect
to have a live object at refcount 0, this causes bugs specially when the object
constructed is complex and has subobjects that get a reference to the parent.

[rickyz (no longer on Chrome), 2015-11-21 00:39:48]

On 2015/11/20 at 22:36:31, cpu wrote:
> And so the refcount starts at 1 so no, we can't use base's code. The issues
with ending ctor at refcount 0 are too long to explain but in general it is
incorrect to have a live object at refcount 0, this causes bugs specially when
the object constructed is complex and has subobjects that get a reference to the
parent.

Assuming that users follow the convention of immediately putting a new object
into scoped_refptr (we can enforce this by making the constructor private), the
only code that can ever run while the object is live and has a refcount of 0 is
the constructor. Is this the situation that you were referring to?

Object::Object() {
  ...
  sub_object_.DoSomething(make_ref_ptr(this));  // releases the only ref and
deletes this before the constructor finishes running
  ...
}

I'd argue that this goes against the rule of avoiding nontrivial work in
constructors (and also potentially passes a partially constructed object to some
nontrivial code, which is pretty questionable). For subobjects, I also prefer
that they take a raw pointer instead of reffing the object, since the object is
guaranteed to outlive the subobject.

Apologies if this is a strawman - this was the only gotcha to using
base::RefCounted that I could think of.

By the way, it's not pretty, but if you're really worried about having a
refcount of 0 in the constructor, we can work around it like this:

class Object : base::RefCounted<Object> {
 public:
  static scoped_refptr<Object> Create();

 private:
  Object();
  ...
};

Object::Object() {
  AddRef();  // released in Object::Create
  ...
}

scoped_refptr<Object> Object::Create() {
  scoped_refptr<Object> obj(new Object);
  obj->Release();  // added in Object::Object
  return obj.Pass();
}

[cpu_(ooo_6.6-7.5), 2015-12-01 00:17:31]

regarless of if some ctor does more work than is blessed by the rules, which btw
is pretty subjective you are adding quite a few 'musts' in order to use
something that is broken in base.

Btw, base/ is in this alone; webkit and mojo both start their refcounts at 1.

https://github.com/domokit/mojo/blob/master/mojo/edk/util/ref_counted_internal.h

I encourage you to fix base/ you will do much good and probably right more than
a few wrongs.

[rickyz (no longer on Chrome), 2015-12-10 02:42:46]

On 2015/12/01 at 00:17:31, cpu wrote:
> regarless of if some ctor does more work than is blessed by the rules, which
btw is pretty subjective you are adding quite a few 'musts' in order to use
something that is broken in base.
> 
> Btw, base/ is in this alone; webkit and mojo both start their refcounts at 1.
> 
>
https://github.com/domokit/mojo/blob/master/mojo/edk/util/ref_counted_internal.h
> 
> I encourage you to fix base/ you will do much good and probably right more
than a few wrongs.

(Sorry for the slow response - just returned from a trip)

I think you've convinced me to initialize the refcount with 1 if I ever need to
implement refcounting, but does this set of CLs need to block on changing base?
I took a quick look at what it would take to change base, and it looks to be a
pretty daunting task - one big challenge would be distinguishing whether each of
the several thousand uses of scoped_refptr is taking the initial reference on an
object (which would need to be replaced with some sort of adopt ref variant) or
whether it is holding an additional reference on an object.

[cpu_(ooo_6.6-7.5), 2015-12-11 18:56:08]

Description was changed from

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
==========

to

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
==========

[cpu_(ooo_6.6-7.5), 2015-12-11 18:56:08]

cpu@chromium.org changed reviewers:
+ vtl@google.com

[cpu_(ooo_6.6-7.5), 2015-12-11 18:56:58]

I think a good way to go is to model it based on mojo's. They are transitioning
from Chrome's to theirs.

[cpu_(ooo_6.6-7.5), 2015-12-11 18:58:20]

btw I added vtl@ in case you have questions about mojo refcount.

As for blocking this CL, no unless you want to use base's refcount then yes.

[vtl, 2015-12-11 18:59:35]

On 2015/12/11 18:58:20, cpu wrote:
> btw I added vtl@ in case you have questions about mojo refcount.
> 
> As for blocking this CL, no unless you want to use base's refcount then yes.

FYI: I think cpu@ is talking about the ref_* files in
https://github.com/domokit/mojo/tree/master/mojo/edk/util .

[cpu_(ooo_6.6-7.5), 2015-12-11 19:02:11]

anyhow without base's refcount which I thought you added but I don't see it
lgtm.

[rickyz (no longer on Chrome), 2015-12-12 00:37:47]

The CQ bit was checked by rickyz@chromium.org

[rickyz (no longer on Chrome), 2015-12-12 00:37:47]

Thanks - the base refcounting stuff was in a dependent CL:
https://codereview.chromium.org/1378523002

We should probably move the discussion there, but just wanted to ask - given the
massive amount of work it would take to change base, what do you think about the
workaround in #12? It should fully address the concern of having a refcount of 0
in the constructor.

[rickyz (no longer on Chrome), 2015-12-12 00:37:47]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1456663003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-12 00:38:47]

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

[commit-bot: I haz the power, 2015-12-12 02:19:51]

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

[commit-bot: I haz the power, 2015-12-12 02:19:52]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rickyz (no longer on Chrome), 2015-12-12 02:23:22]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-12-12 02:26:54]

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

[commit-bot: I haz the power, 2015-12-12 02:44:29]

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

[commit-bot: I haz the power, 2015-12-12 02:44:31]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[rickyz (no longer on Chrome), 2015-12-12 02:45:10]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-12-12 02:48:00]

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

[commit-bot: I haz the power, 2015-12-12 04:34:44]

Description was changed from

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
==========

to

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
==========

[commit-bot: I haz the power, 2015-12-12 04:34:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-12 04:35:48]

Description was changed from

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319
==========

to

==========
Make dispatcher a member of PolicyBase.

This is the first step of unifying PolicyBase and TargetPolicy, which
should let us get rid of the casting we do between these two.

BUG=549319

Committed: https://crrev.com/60f931dc587fee8afbc933ec2d2fac8b24506eed
Cr-Commit-Position: refs/heads/master@{#364904}
==========

[commit-bot: I haz the power, 2015-12-12 04:35:50]

Patchset 5 (id:??) landed as
https://crrev.com/60f931dc587fee8afbc933ec2d2fac8b24506eed
Cr-Commit-Position: refs/heads/master@{#364904}