Oilpan: Reapply: Remove all the RefPtrs and most of the raw pointers to style

Oilpan: Reapply: Remove all the RefPtrs and most of the raw pointers to style
sheets.

Comment on the cases where the raw pointers cannot be removed at this point.

Leave the raw pointers in inspector/ alove. They are safe as they are
now because the inspector leaks all the CSSStyleSheets it is inspecting
via Persistent handles at this point (and used to leak them all via
RefPtrs.)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169462

[Mads Ager (chromium), 2014-03-17 13:57:23]

Patch set 2 is the changes that I undid again. They were all changes that I did
after review comments. I should have thought them through and run tests before
committing. Now we are back at something that works. :)

https://codereview.chromium.org/196513007/diff/1/Source/core/css/ElementRuleC...
File Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/196513007/diff/1/Source/core/css/ElementRuleC...
Source/core/css/ElementRuleCollector.h:76: RawPtrWillBeMember<const RuleData>
m_ruleData;
This change should have been caught by the COMPILE_ASSERT in the Member
constructor. Unfortunately, those don't really work at this point. I'm fixing
them so they will catch issues like these.

https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
File Source/core/dom/ShadowTreeStyleSheetCollection.h (right):

https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
Source/core/dom/ShadowTreeStyleSheetCollection.h:41: class
ShadowTreeStyleSheetCollection FINAL : public
NoBaseWillBeGarbageCollectedFinalized<ShadowTreeStyleSheetCollection>, public
TreeScopeStyleSheetCollection {
And the errors made me remember why we have to only have this one heap
allocated. The issue is that StyleSheetCollection is stack allocated which we do
not allow for GarbageCollected objects (because it is really dangerous - you can
get an address of one of these into a Member and get memory corruption when you
mark it.

[haraken, 2014-03-17 14:29:37]

https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
File Source/core/dom/ShadowTreeStyleSheetCollection.h (right):

https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
Source/core/dom/ShadowTreeStyleSheetCollection.h:41: class
ShadowTreeStyleSheetCollection FINAL : public
NoBaseWillBeGarbageCollectedFinalized<ShadowTreeStyleSheetCollection>, public
TreeScopeStyleSheetCollection {
On 2014/03/17 13:57:24, Mads Ager (chromium) wrote:
> And the errors made me remember why we have to only have this one heap
> allocated. The issue is that StyleSheetCollection is stack allocated which we
do
> not allow for GarbageCollected objects (because it is really dangerous - you
can
> get an address of one of these into a Member and get memory corruption when
you
> mark it.

Would you elaborate on this? Do you mean we cannot put a GarbageCollected object
on stack?

class A : public GarbageCollected<A> { };

void foo() {
  A a;  // Is this unsafe?
  ...;
}

[Erik Corry, 2014-03-17 14:48:58]

On 2014/03/17 14:29:37, haraken wrote:
>
https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
> File Source/core/dom/ShadowTreeStyleSheetCollection.h (right):
> 
>
https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
> Source/core/dom/ShadowTreeStyleSheetCollection.h:41: class
> ShadowTreeStyleSheetCollection FINAL : public
> NoBaseWillBeGarbageCollectedFinalized<ShadowTreeStyleSheetCollection>, public
> TreeScopeStyleSheetCollection {
> On 2014/03/17 13:57:24, Mads Ager (chromium) wrote:
> > And the errors made me remember why we have to only have this one heap
> > allocated. The issue is that StyleSheetCollection is stack allocated which
we
> do
> > not allow for GarbageCollected objects (because it is really dangerous - you
> can
> > get an address of one of these into a Member and get memory corruption when
> you
> > mark it.
> 
> Would you elaborate on this? Do you mean we cannot put a GarbageCollected
object
> on stack?
> 
> class A : public GarbageCollected<A> { };
> 
> void foo() {
>   A a;  // Is this unsafe?
>   ...;
> }

This is unsafe:

class A : public GarbageCollected<A> { };

class B : public GarbageCollected<B> {
  B(A& a) : m_a(&a) { }
  Member<A> m_a;
};

void foo() {
  A a;
  B* b = new B(a);
  // Do something here that may cause a GC and cause b to get traced.
}

This is dangerous because when we trace the a we will try to set the mark bit on
a, which will be a random memory write.

A good way to avoid this is never to put garbage collected things on the stack.

But we do already violate this when we put HeapVector etc. on the stack.  So we
need to be careful

[Mads Ager (chromium), 2014-03-17 14:50:08]

On 2014/03/17 14:29:37, haraken wrote:
>
https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
> File Source/core/dom/ShadowTreeStyleSheetCollection.h (right):
> 
>
https://codereview.chromium.org/196513007/diff/20001/Source/core/dom/ShadowTr...
> Source/core/dom/ShadowTreeStyleSheetCollection.h:41: class
> ShadowTreeStyleSheetCollection FINAL : public
> NoBaseWillBeGarbageCollectedFinalized<ShadowTreeStyleSheetCollection>, public
> TreeScopeStyleSheetCollection {
> On 2014/03/17 13:57:24, Mads Ager (chromium) wrote:
> > And the errors made me remember why we have to only have this one heap
> > allocated. The issue is that StyleSheetCollection is stack allocated which
we
> do
> > not allow for GarbageCollected objects (because it is really dangerous - you
> can
> > get an address of one of these into a Member and get memory corruption when
> you
> > mark it.
> 
> Would you elaborate on this? Do you mean we cannot put a GarbageCollected
object
> on stack?
> 
> class A : public GarbageCollected<A> { };
> 
> void foo() {
>   A a;  // Is this unsafe?
>   ...;
> }

Yes, that's what it means. We have an assert in the GarbageCollected constructor
that |this| is in the heap. That has caught errors where we ended up with
pointers to objects that were not in our heap in Members. If you take the
address of a in your example and put it in a traced Member you get memory
corruption if a GC gets triggered. Therefore, we are disallowing it.

[haraken, 2014-03-17 17:43:55]

> This is unsafe:
> 
> class A : public GarbageCollected<A> { };
> 
> class B : public GarbageCollected<B> {
>   B(A& a) : m_a(&a) { }
>   Member<A> m_a;
> };
> 
> void foo() {
>   A a;
>   B* b = new B(a);
>   // Do something here that may cause a GC and cause b to get traced.
> }
> 
> This is dangerous because when we trace the a we will try to set the mark bit
on
> a, which will be a random memory write.
> 
> A good way to avoid this is never to put garbage collected things on the
stack.
> 
> But we do already violate this when we put HeapVector etc. on the stack.  So
we
> need to be careful

Yeah, that's why I'm asking this. It sounds inconsistent that we can put some
GarbageCollected objects on a stack but cannot put some GarbageCollected objects
on a stack. Specifically:

- We can put HeapVector etc on a stack. However, we have to be careful that we
don't take the address of the stack-allocated HeapVector and store the address
into a Member of on-heap objects.
- We cannot put other GarbageCollected objects on a stack. This is for avoiding
us from taking the address of the stack-allocated object.

This rule sounds a bit inconsistent to me.

IMHO, we can just allow GarbageCollected objects on a stack, assuming that no
one will store the address of a stack-allocated object into an on-heap object. I
cannot come up with valid code which stores the address of a stack-allocated
object into an on-heap object (even without oilpan).

Mads@: Specifically, what error did you hit when you make StyleSheetCollection
GarbageCollected?

[Mads Ager (chromium), 2014-03-18 06:17:37]

On 2014/03/17 17:43:55, haraken wrote:
> > This is unsafe:
> > 
> > class A : public GarbageCollected<A> { };
> > 
> > class B : public GarbageCollected<B> {
> >   B(A& a) : m_a(&a) { }
> >   Member<A> m_a;
> > };
> > 
> > void foo() {
> >   A a;
> >   B* b = new B(a);
> >   // Do something here that may cause a GC and cause b to get traced.
> > }
> > 
> > This is dangerous because when we trace the a we will try to set the mark
bit
> on
> > a, which will be a random memory write.
> > 
> > A good way to avoid this is never to put garbage collected things on the
> stack.
> > 
> > But we do already violate this when we put HeapVector etc. on the stack.  So
> we
> > need to be careful
> 
> Yeah, that's why I'm asking this. It sounds inconsistent that we can put some
> GarbageCollected objects on a stack but cannot put some GarbageCollected
objects
> on a stack. Specifically:
> 
> - We can put HeapVector etc on a stack. However, we have to be careful that we
> don't take the address of the stack-allocated HeapVector and store the address
> into a Member of on-heap objects.
> - We cannot put other GarbageCollected objects on a stack. This is for
avoiding
> us from taking the address of the stack-allocated object.
> 
> This rule sounds a bit inconsistent to me.
>
> IMHO, we can just allow GarbageCollected objects on a stack, assuming that no
> one will store the address of a stack-allocated object into an on-heap object.
I
> cannot come up with valid code which stores the address of a stack-allocated
> object into an on-heap object (even without oilpan).

That is actually very inconsistent. I hadn't thought about the heap collection
case. There is nothing wrong with having the object on the stack, that is safe
and stack scanning will get it properly traced. It is only if you end up passing
the address somewhere and putting it in a Member that it becomes unsafe. One
case that I could imagine happening is passing stack allocated objects to the
constructor of other stack allocated object that puts it in a Member:

{
    StackAllocGarbageCollected a;
    DoSomethingScope scope(&a);
}

So, I do think this is dangerous. On the other hand, the kind of code that I
have in this changelist is dangerous too and does not follow our usual pattern
at all.

Alright, I'll allow these objects on the stack and we need to be careful with
how we use them. We should see if we can find a way to help us verify correct
usage of stack-allocated Heap collections and garbage collected objects.

> Mads@: Specifically, what error did you hit when you make StyleSheetCollection
> GarbageCollected?

It was just the assert in the GarbageCollected constructor.

The real problem with the changelist that I landed was that I put the RuleData*
in a Member. Since RuleData is not garbage collected, that leads to memory
corruption (in the same way that the scenario above with the stack allocated
object in a Member does - we end up marking an object which messes with memory
just before the object.) I landed a change yesterday to fix the compile-time
asserts so that this will not happen again. :)

[Mads Ager (chromium), 2014-03-18 06:29:11]

Uploaded new change. I move the heap contains assert from the GarbageCollected
constructor to the Member (and thereby WeakMember) and Persistent constructors.
That way at least we get assertion failures in debug mode if we put a stack
allocated object in a Member or a Persistent but we do allow objects that
inherit from GarbageCollected being stack allocated.

PTAL

[haraken, 2014-03-18 09:40:32]

Thanks for the update! LGTM.

[Mads Ager (chromium), 2014-03-18 13:48:23]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 13:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/196513007/90001

[commit-bot: I haz the power, 2014-03-18 14:54:44]

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

[commit-bot: I haz the power, 2014-03-18 14:54:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Mads Ager (chromium), 2014-03-18 16:12:01]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 16:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/196513007/90001

[commit-bot: I haz the power, 2014-03-18 17:20:19]

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

[commit-bot: I haz the power, 2014-03-18 17:20:22]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Mads Ager (chromium), 2014-03-18 17:24:57]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:25:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/196513007/90001

[commit-bot: I haz the power, 2014-03-18 19:09:19]

Change committed as 169462