Consistently use on-heap collections for StyleRuleBase descendants.

Consistently use on-heap collections for StyleRuleBase descendants.

StyleRuleBase and StyleSheetContents are on the heap now so off heap collections of raw pointers to them are disallowed.

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

[Vyacheslav Egorov (Chromium), 2014-02-25 15:58:22]

PTAL

[sof, 2014-02-25 16:18:00]

https://codereview.chromium.org/180003002/diff/1/Source/core/css/StyleSheetCo...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/1/Source/core/css/StyleSheetCo...
Source/core/css/StyleSheetContents.h:168: Member<StyleRuleImport> m_ownerRule;
RawPtrWillBeMember?

[Vyacheslav Egorov (Chromium), 2014-02-25 16:22:27]

https://codereview.chromium.org/180003002/diff/1/Source/core/css/StyleSheetCo...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/1/Source/core/css/StyleSheetCo...
Source/core/css/StyleSheetContents.h:168: Member<StyleRuleImport> m_ownerRule;
On 2014/02/25 16:18:01, sof wrote:
> RawPtrWillBeMember?

Done.

[I want oilpan enabled asap :-)]

[Mads Ager (chromium), 2014-02-25 19:35:58]

LGTM

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchResult.h:70: struct MatchResult {
STACK_ALLOCATED? (If Ian's change has landed?)

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchResult.h:73: WillBeHeapVector<StyleRule*, 64>
matchedRules;
WillBeHeapVector<RawPtrWillBeMember<StyleRule>, 64> ?

[haraken, 2014-02-26 02:20:12]

This CL is changing a lot of raw pointers with Persistents. I'm a bit afraid
that it will cause leaks in this CL or cause a seed of leaks that are triggered
in the future.

(That being said, IMHO, I think it would be sometimes hard to ensure that each
incremental oilpan change won't cause leaks. I'm OK as long as the leaks are
fixed as a consequence of moving the entire hierarchy.)

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
File Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:78: // FIXME: when transition types are
gone this class can be replaced with HeapVector.

FIXME: oilpan:

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:82:
WillBePersistentHeapVector<StyleRule*> m_list;

This should be WillBePersistentHeapVector<RawPtrWillBeMember<StyleRule>>.
Otherwise StyleRules are not traced, right?

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:82:
WillBePersistentHeapVector<StyleRule*> m_list;

Are you sure that this won't produce memory leaks? This is changing raw pointers
to Persistents.

You might want to move StyleRuleList to the heap at the same time if it isn't
that hard. (I'm not sure if moving StyleRuleList will fix the leak, but changing
raw pointers with Members will be "better" than changing raw pointers with
Persistents.)

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
File Source/core/css/PageRuleCollector.cpp (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
Source/core/css/PageRuleCollector.cpp:76:
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> > matchedPageRules;

You might want to introduce typedef for
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage>. For example,
StyleRulePageVector.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }

Just to confirm: It is safe to convert PersistentHeapVector to HeapVector,
right?

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:212:
WillBePersistentHeapVector<RawPtrWillBeMember<StyleRuleKeyframes> >
m_keyframesRules;

These are changing raw pointers with Persistents. Make sure that these won't
cause leaks.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.h:117: void clearOwnerRule() { m_ownerRule =
nullptr; }

This is called from ~StyleRuleImport(). This is dangerous, since StyleRuleImport
is already on the heap. Probably you can remove clearOwnerRule() from
~StyleRuleImport().

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScopeStyleSheetCollection.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
Source/core/dom/TreeScopeStyleSheetCollection.h:89: DISALLOW_ALLOCATION();

Let's put this at the head of the struct.

Probably we want to add 'public:' to the end of the DISALLOW_ALLOCATION macro so
that we don't need to write 'public:' after the macro.

https://codereview.chromium.org/180003002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1307: HeapVector(size_t size, const T& val) : Vector<T,
inlineCapacity, HeapAllocator>(size, val)

We might want to add a test for this.

[Erik Corry, 2014-02-26 13:01:28]

LGTM

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
File Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:82:
WillBePersistentHeapVector<StyleRule*> m_list;
On 2014/02/26 02:20:13, haraken wrote:
> 
> Are you sure that this won't produce memory leaks? This is changing raw
pointers
> to Persistents.
> 
> You might want to move StyleRuleList to the heap at the same time if it isn't
> that hard. (I'm not sure if moving StyleRuleList will fix the leak, but
changing
> raw pointers with Members will be "better" than changing raw pointers with
> Persistents.)

If the StyleRule stays raw, then it will not leak (the persistent will just keep
the backing store alive).

Presumably there is currently some property of pre-oilpan Blink that ensures
these are not collected during the lifetime of m_list.  When this is moved to
the heap and becomes HeapVector it will be natural to make these raw pointer
Members.

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScopeStyleSheetCollection.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
Source/core/dom/TreeScopeStyleSheetCollection.h:89: DISALLOW_ALLOCATION();
On 2014/02/26 02:20:13, haraken wrote:
> 
> Let's put this at the head of the struct.
> 
> Probably we want to add 'public:' to the end of the DISALLOW_ALLOCATION macro
so
> that we don't need to write 'public:' after the macro.

Classes normally start off private, so it would be strange if there was a hidden
"public:" inside a macro used at the start.

[Vyacheslav Egorov (Chromium), 2014-02-26 13:12:23]

PTAL

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
File Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:78: // FIXME: when transition types are
gone this class can be replaced with HeapVector.
On 2014/02/26 02:20:13, haraken wrote:
> 
> FIXME: oilpan:

Done.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/ElementR...
Source/core/css/ElementRuleCollector.h:82:
WillBePersistentHeapVector<StyleRule*> m_list;
On 2014/02/26 02:20:13, haraken wrote:
> 
> This should be WillBePersistentHeapVector<RawPtrWillBeMember<StyleRule>>.
> Otherwise StyleRules are not traced, right?

Right thanks. I really don't like verbosity of the Collection<Member<T> > so I
always forget to write Member there.

On 2014/02/26 02:20:13, haraken wrote:
> 
> Are you sure that this won't produce memory leaks? This is changing raw
pointers
> to Persistents.
> 

StyleRuleList is owned by stack allocated ElementRuleCollector, so no leaks will
be introduced.

> You might want to move StyleRuleList to the heap at the same time if it isn't
> that hard. (I'm not sure if moving StyleRuleList will fix the leak, but
changing
> raw pointers with Members will be "better" than changing raw pointers with
> Persistents.)

I tried replacing StyleRuleList with HeapVector if ENABLE(OILPAN) as moving it
to the heap does not make much sense in terms of redundancy. However I did not
find a good way to avoid ifdef's at use sites.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
File Source/core/css/PageRuleCollector.cpp (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
Source/core/css/PageRuleCollector.cpp:76:
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> > matchedPageRules;
On 2014/02/26 02:20:13, haraken wrote:
> 
> You might want to introduce typedef for
> WillBeHeapVector<RawPtrWillBeMember<StyleRulePage>. For example,
> StyleRulePageVector.

It's somewhat obscure to have these typedef's because underlying data structure
names goes to the end instead of staying in-front. 

Though it helps to cut down the verbosity so I am not sure what I really would
prefer.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }
On 2014/02/26 02:20:13, haraken wrote:
> 
> Just to confirm: It is safe to convert PersistentHeapVector to HeapVector,
> right?

It is safe to convert PersistentHeapVector& to HeapVector& because HeapVector is
a base class for PersistentHeapVector.

Converting PersistentHeapVector to HeapVector would require copying but it is
also safe as long as we store resulting HeapVector on the stack on in some place
where we trace through it.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:212:
WillBePersistentHeapVector<RawPtrWillBeMember<StyleRuleKeyframes> >
m_keyframesRules;
On 2014/02/26 02:20:13, haraken wrote:
> 
> These are changing raw pointers with Persistents. Make sure that these won't
> cause leaks.

On the first glance it does not seem like it introduces leaks, but these
persistents will disappear when we move RuleSet to the heap. 

I think some leaks are unavoidable if we want to keep CLs smalls. The only way
to move stuff without any leaks would be to submit one CL that moves everything
in :-)

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.h:117: void clearOwnerRule() { m_ownerRule =
nullptr; }
On 2014/02/26 02:20:13, haraken wrote:
> 
> This is called from ~StyleRuleImport(). This is dangerous, since
StyleRuleImport
> is already on the heap. Probably you can remove clearOwnerRule() from
> ~StyleRuleImport().

Good catch! I asked people to pay attention to finalizers and forgot to check
this myself.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchResult.h:70: struct MatchResult {
On 2014/02/25 19:35:58, Mads Ager (chromium) wrote:
> STACK_ALLOCATED? (If Ian's change has landed?)

Done.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchResult.h:73: WillBeHeapVector<StyleRule*, 64>
matchedRules;
On 2014/02/25 19:35:58, Mads Ager (chromium) wrote:
> WillBeHeapVector<RawPtrWillBeMember<StyleRule>, 64> ?

Done.

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScopeStyleSheetCollection.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/dom/TreeScop...
Source/core/dom/TreeScopeStyleSheetCollection.h:89: DISALLOW_ALLOCATION();
On 2014/02/26 02:20:13, haraken wrote:
> 
> Let's put this at the head of the struct.
> 
> Probably we want to add 'public:' to the end of the DISALLOW_ALLOCATION macro
so
> that we don't need to write 'public:' after the macro.

Done.

https://codereview.chromium.org/180003002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1307: HeapVector(size_t size, const T& val) : Vector<T,
inlineCapacity, HeapAllocator>(size, val)
On 2014/02/26 02:20:13, haraken wrote:
> 
> We might want to add a test for this.

Done.

[haraken, 2014-02-26 13:23:48]

Thanks for the update! LGTM.

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
File Source/core/css/PageRuleCollector.cpp (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/PageRule...
Source/core/css/PageRuleCollector.cpp:76:
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> > matchedPageRules;
On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> On 2014/02/26 02:20:13, haraken wrote:
> > 
> > You might want to introduce typedef for
> > WillBeHeapVector<RawPtrWillBeMember<StyleRulePage>. For example,
> > StyleRulePageVector.
> 
> It's somewhat obscure to have these typedef's because underlying data
structure
> names goes to the end instead of staying in-front. 
> 
> Though it helps to cut down the verbosity so I am not sure what I really would
> prefer.

Yeah, we decided not to use typedef since it makes underlying data structure
obscure and cause unexpected bugs (forget tracing etc).

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }
On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> On 2014/02/26 02:20:13, haraken wrote:
> > 
> > Just to confirm: It is safe to convert PersistentHeapVector to HeapVector,
> > right?
> 
> It is safe to convert PersistentHeapVector& to HeapVector& because HeapVector
is
> a base class for PersistentHeapVector.
> 
> Converting PersistentHeapVector to HeapVector would require copying but it is
> also safe as long as we store resulting HeapVector on the stack on in some
place
> where we trace through it.

It's sad it requires copying.

Probably shall we add operator*() that retrieves HeapVector from
PersistentHeapVector?

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:212:
WillBePersistentHeapVector<RawPtrWillBeMember<StyleRuleKeyframes> >
m_keyframesRules;
On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> On 2014/02/26 02:20:13, haraken wrote:
> > 
> > These are changing raw pointers with Persistents. Make sure that these won't
> > cause leaks.
> 
> On the first glance it does not seem like it introduces leaks, but these
> persistents will disappear when we move RuleSet to the heap. 
> 
> I think some leaks are unavoidable if we want to keep CLs smalls. The only way
> to move stuff without any leaks would be to submit one CL that moves
everything
> in :-)

Agreed. I'm OK as long as the leak is fixed until we enable the oilpan flag :)

https://codereview.chromium.org/180003002/diff/40001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/40001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.h:117: void clearOwnerRule() { m_ownerRule =
nullptr; }

Shall we add #if ENABLE(OILPAN) ?

[Vyacheslav Egorov (Chromium), 2014-02-26 13:47:11]

https://codereview.chromium.org/180003002/diff/40001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/180003002/diff/40001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.h:117: void clearOwnerRule() { m_ownerRule =
nullptr; }
On 2014/02/26 13:23:49, haraken wrote:
> 
> Shall we add #if ENABLE(OILPAN) ?

setCSSStyleSheet() calls this so I can't do that.

[Vyacheslav Egorov (Chromium), 2014-02-26 13:47:15]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-02-26 13:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/180003002/40001

[haraken, 2014-02-26 13:47:59]

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }
On 2014/02/26 13:23:49, haraken wrote:
> On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> > On 2014/02/26 02:20:13, haraken wrote:
> > > 
> > > Just to confirm: It is safe to convert PersistentHeapVector to HeapVector,
> > > right?
> > 
> > It is safe to convert PersistentHeapVector& to HeapVector& because
HeapVector
> is
> > a base class for PersistentHeapVector.
> > 
> > Converting PersistentHeapVector to HeapVector would require copying but it
is
> > also safe as long as we store resulting HeapVector on the stack on in some
> place
> > where we trace through it.
> 
> It's sad it requires copying.
> 
> Probably shall we add operator*() that retrieves HeapVector from
> PersistentHeapVector?

Note: The same issue is happening in https://codereview.chromium.org/180003002/.
Class X holds PersistentHeapVector but X::foo() wants to return HeapVector.
Probably we might want to add operator*() that does the retrieval.

[Vyacheslav Egorov (Chromium), 2014-02-26 14:13:28]

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }
Returning a *reference* does not require copying (e.g. there is no copying in
this case) returning by value of course requires a copying, just like returning
any Vector by value. Maybe my comment was confusing.

On 2014/02/26 13:48:00, haraken wrote:
> On 2014/02/26 13:23:49, haraken wrote:
> > On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> > > On 2014/02/26 02:20:13, haraken wrote:
> > > > 
> > > > Just to confirm: It is safe to convert PersistentHeapVector to
HeapVector,
> > > > right?
> > > 
> > > It is safe to convert PersistentHeapVector& to HeapVector& because
> HeapVector
> > is
> > > a base class for PersistentHeapVector.
> > > 
> > > Converting PersistentHeapVector to HeapVector would require copying but it
> is
> > > also safe as long as we store resulting HeapVector on the stack on in some
> > place
> > > where we trace through it.
> > 
> > It's sad it requires copying.
> > 
> > Probably shall we add operator*() that retrieves HeapVector from
> > PersistentHeapVector?
> 
> Note: The same issue is happening in
https://codereview.chromium.org/180003002/.
> Class X holds PersistentHeapVector but X::foo() wants to return HeapVector.
> Probably we might want to add operator*() that does the retrieval.

[haraken, 2014-02-26 14:20:53]

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/180003002/diff/20001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:136: const
WillBeHeapVector<RawPtrWillBeMember<StyleRulePage> >& pageRules() const {
ASSERT(!m_pendingRules); return m_pageRules; }
On 2014/02/26 14:13:28, Vyacheslav Egorov wrote:
> Returning a *reference* does not require copying (e.g. there is no copying in
> this case) returning by value of course requires a copying, just like
returning
> any Vector by value. Maybe my comment was confusing.
> 
> On 2014/02/26 13:48:00, haraken wrote:
> > On 2014/02/26 13:23:49, haraken wrote:
> > > On 2014/02/26 13:12:24, Vyacheslav Egorov wrote:
> > > > On 2014/02/26 02:20:13, haraken wrote:
> > > > > 
> > > > > Just to confirm: It is safe to convert PersistentHeapVector to
> HeapVector,
> > > > > right?
> > > > 
> > > > It is safe to convert PersistentHeapVector& to HeapVector& because
> > HeapVector
> > > is
> > > > a base class for PersistentHeapVector.
> > > > 
> > > > Converting PersistentHeapVector to HeapVector would require copying but
it
> > is
> > > > also safe as long as we store resulting HeapVector on the stack on in
some
> > > place
> > > > where we trace through it.
> > > 
> > > It's sad it requires copying.
> > > 
> > > Probably shall we add operator*() that retrieves HeapVector from
> > > PersistentHeapVector?
> > 
> > Note: The same issue is happening in
> https://codereview.chromium.org/180003002/.
> > Class X holds PersistentHeapVector but X::foo() wants to return HeapVector.
> > Probably we might want to add operator*() that does the retrieval.
> 

ah, makes sense, thanks! (though I think we'll need operator* at some point.)

[commit-bot: I haz the power, 2014-02-26 16:03:10]

Change committed as 167916