Move CSSRuleList to the garbage collected heap

Move CSSRuleList to the garbage collected heap

R=ager@chromium.org, haraken@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=

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

[Mads Ager (chromium), 2014-03-10 12:32:56]

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.cpp (left):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.cpp:41: if (m_styleSheetCSSOMWrapper)
Uh, careful. We probably still need this when !ENABLE(OILPAN)?

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.h (right):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.h:59: mutable RefPtrWillBeMember<CSSStyleSheet>
m_styleSheetCSSOMWrapper;
Thanks. I'm doing the same change as we speak.

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

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRule....
Source/core/css/CSSRule.h:37: // FIXME: oilpan: CSSRuleList redirects increments
and decrements of its
Remove deprecated comment.

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:88: // The rule owns the live list. FIXME: oilpan:
Delete this comment.
Let's just remove the comment now. The fact that you now have a
PassOwnPtrWillBeRawPtr makes it clear that something has this list in an OwnPtr.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:51: virtual void trace(Visitor*) { }
Make this a pure virtual as well? (Either that or call it in subclasses
explicitly but there is no point in that.)

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:104: CSSRuleList::trace(visitor);
If you make it pure virtual you can leave this one out.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:54: visitor->trace(m_styleSheet);
If you do not make CSSRuleList::trace pure virtual you should make sure to call
it here.

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:74: class StyleEngine : public
NoBaseWillBeGarbageCollectedFinalized<StyleEngine> {
This will lead to finalization-order issues. You need to introduce a
detachedFromDocument method that will carry out the work of the StyleEngine
destructor. Then you need an empty StyleEngine destructor. You have to call
detachedFromDocument on the style engine in the Document destructor.

[zerny-chromium, 2014-03-10 14:00:20]

Mostly lgtm

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.cpp:59: CSSRuleList::trace(visitor);
This one can be left out too if the base is pure-virtual.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:51: virtual void trace(Visitor*) { }
On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> Make this a pure virtual as well? (Either that or call it in subclasses
> explicitly but there is no point in that.)

The plugin should warn that this trace is not called from the derived classes.
Making it pure-virtual as Mads suggests will resolve the issue.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetList.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/StyleShe...
Source/core/css/StyleSheetList.cpp:94: m_detachedStyleSheets.trace(visitor);
Nit: I think the convention is: visitor->trace(m_detachedStyleSheets)

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:221: mutable
WillBePersistentHeapVector<RefPtrWillBeMember<CSSStyleSheet> >
m_injectedAuthorStyleSheets;
Why is this a persistent and not just a traced heap vector?

[Erik Corry, 2014-03-12 10:36:40]

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.cpp:59: CSSRuleList::trace(visitor);
On 2014/03/10 14:00:21, zerny-chromium wrote:
> This one can be left out too if the base is pure-virtual.

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:51: virtual void trace(Visitor*) { }
On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> Make this a pure virtual as well? (Either that or call it in subclasses
> explicitly but there is no point in that.)

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:51: virtual void trace(Visitor*) { }
On 2014/03/10 14:00:21, zerny-chromium wrote:
> On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> > Make this a pure virtual as well? (Either that or call it in subclasses
> > explicitly but there is no point in that.)
> 
> The plugin should warn that this trace is not called from the derived classes.
> Making it pure-virtual as Mads suggests will resolve the issue.

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:104: CSSRuleList::trace(visitor);
On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> If you make it pure virtual you can leave this one out.

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:54: visitor->trace(m_styleSheet);
On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> If you do not make CSSRuleList::trace pure virtual you should make sure to
call
> it here.

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetList.cpp (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/css/StyleShe...
Source/core/css/StyleSheetList.cpp:94: m_detachedStyleSheets.trace(visitor);
On 2014/03/10 14:00:21, zerny-chromium wrote:
> Nit: I think the convention is: visitor->trace(m_detachedStyleSheets)

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:74: class StyleEngine : public
NoBaseWillBeGarbageCollectedFinalized<StyleEngine> {
On 2014/03/10 12:32:57, Mads Ager (chromium) wrote:
> This will lead to finalization-order issues. You need to introduce a
> detachedFromDocument method that will carry out the work of the StyleEngine
> destructor. Then you need an empty StyleEngine destructor. You have to call
> detachedFromDocument on the style engine in the Document destructor.

Done.

https://codereview.chromium.org/192473003/diff/40001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:221: mutable
WillBePersistentHeapVector<RefPtrWillBeMember<CSSStyleSheet> >
m_injectedAuthorStyleSheets;
On 2014/03/10 14:00:21, zerny-chromium wrote:
> Why is this a persistent and not just a traced heap vector?

fixed

[haraken, 2014-03-12 11:08:20]

LGTM with comments.

https://codereview.chromium.org/192473003/diff/60001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetList.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/css/StyleShe...
Source/core/css/StyleSheetList.h:57: TreeScope* m_treeScope;

This raw pointer looks safe since it's cleared in detachFromDocument().

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:561: m_styleEngine->detachFromDocument();

Don't you need to clear m_styleEngine after calling detachFromDocument() ?

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
File Source/core/dom/DocumentStyleSheetCollector.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollector.h:43: // This class contains
references to two on-heap collections, therefore

We might want to change the references to Members in a follow-up CL.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollector.h:45: // scanning wil keep them
alive.

wil => will

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:120: m_styleSheetCollectionMap.clear();

Don't you need to clear m_document, m_dirtyTreeScopes and m_activeTreeScopes as
well ?

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:640: m_injectedAuthorStyleSheets.trace(visitor);

visitor->trace(m_injectedAuthorStyleSheets)

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:641: m_authorStyleSheets.trace(visitor);

visitor->trace(m_authorStyleSheets);

https://codereview.chromium.org/192473003/diff/60001/Source/core/storage/Stor...
File Source/core/storage/StorageEvent.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/storage/Stor...
Source/core/storage/StorageEvent.h:46: RefPtrWillBeRawPtr<Storage> storageArea;

This should be RefPtrWillBeMember.

We'll need to sweep the code base and replace RawPtrs in stack-allocated classes
with Members :)

https://codereview.chromium.org/192473003/diff/60001/Source/core/xml/XSLStyle...
File Source/core/xml/XSLStyleSheet.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/xml/XSLStyle...
Source/core/xml/XSLStyleSheet.h:29: #include "heap/Heap.h"

Handle.h?

[Erik Corry, 2014-03-12 11:48:56]

https://codereview.chromium.org/192473003/diff/60001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetList.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/css/StyleShe...
Source/core/css/StyleSheetList.h:57: TreeScope* m_treeScope;
On 2014/03/12 11:08:21, haraken wrote:
> 
> This raw pointer looks safe since it's cleared in detachFromDocument().

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:561: m_styleEngine->detachFromDocument();
On 2014/03/12 11:08:21, haraken wrote:
> 
> Don't you need to clear m_styleEngine after calling detachFromDocument() ?

The clearing was just to deref it, as I understand.  This is the destructor, so
it gets dereffed at the end anyway, and any clearing will not be observable
after that point.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
File Source/core/dom/DocumentStyleSheetCollector.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollector.h:43: // This class contains
references to two on-heap collections, therefore
On 2014/03/12 11:08:21, haraken wrote:
> 
> We might want to change the references to Members in a follow-up CL.

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollector.h:45: // scanning wil keep them
alive.
On 2014/03/12 11:08:21, haraken wrote:
> 
> wil => will

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:120: m_styleSheetCollectionMap.clear();
On 2014/03/12 11:08:21, haraken wrote:
> 
> Don't you need to clear m_document, m_dirtyTreeScopes and m_activeTreeScopes
as
> well ?

I'm clearing the RefPtrs and the OwnPtrs so that their finalizers get called
now.  m_document is a C++ reference so I can't clear it, and the are collections
with no finalizers (raw pointers), so it would have no effect.

I'll add a comment.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:640: m_injectedAuthorStyleSheets.trace(visitor);
On 2014/03/12 11:08:21, haraken wrote:
> 
> visitor->trace(m_injectedAuthorStyleSheets)

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:641: m_authorStyleSheets.trace(visitor);
On 2014/03/12 11:08:21, haraken wrote:
> 
> visitor->trace(m_authorStyleSheets);

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/storage/Stor...
File Source/core/storage/StorageEvent.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/storage/Stor...
Source/core/storage/StorageEvent.h:46: RefPtrWillBeRawPtr<Storage> storageArea;
On 2014/03/12 11:08:21, haraken wrote:
> 
> This should be RefPtrWillBeMember.
> 
> We'll need to sweep the code base and replace RawPtrs in stack-allocated
classes
> with Members :)

Done.

https://codereview.chromium.org/192473003/diff/60001/Source/core/xml/XSLStyle...
File Source/core/xml/XSLStyleSheet.h (right):

https://codereview.chromium.org/192473003/diff/60001/Source/core/xml/XSLStyle...
Source/core/xml/XSLStyleSheet.h:29: #include "heap/Heap.h"
On 2014/03/12 11:08:21, haraken wrote:
> 
> Handle.h?

Removed.

[Mads Ager (chromium), 2014-03-12 12:24:41]

LGTM

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.cpp (left):

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.cpp:41: if (m_styleSheetCSSOMWrapper)
I think you still need this when !ENABLE(OILPAN).

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:88: // The rule owns the live list. FIXME: oilpan:
Delete this comment.
Let's just remove this comment. The fact that OwnPtrs are used should signal
that this is owned by the rule.

Did you look at my first round of comments? ;-)

[Erik Corry, 2014-03-12 12:31:26]

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.cpp (left):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.cpp:41: if (m_styleSheetCSSOMWrapper)
On 2014/03/10 12:32:56, Mads Ager (chromium) wrote:
> Uh, careful. We probably still need this when !ENABLE(OILPAN)?

Done.

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.h (right):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.h:59: mutable RefPtrWillBeMember<CSSStyleSheet>
m_styleSheetCSSOMWrapper;
On 2014/03/10 12:32:56, Mads Ager (chromium) wrote:
> Thanks. I'm doing the same change as we speak.

OK

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

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRule....
Source/core/css/CSSRule.h:37: // FIXME: oilpan: CSSRuleList redirects increments
and decrements of its
On 2014/03/10 12:32:56, Mads Ager (chromium) wrote:
> Remove deprecated comment.

Done.

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/20001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:88: // The rule owns the live list. FIXME: oilpan:
Delete this comment.
On 2014/03/10 12:32:56, Mads Ager (chromium) wrote:
> Let's just remove the comment now. The fact that you now have a
> PassOwnPtrWillBeRawPtr makes it clear that something has this list in an
OwnPtr.

Done.

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSImpor...
File Source/core/css/CSSImportRule.cpp (left):

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSImpor...
Source/core/css/CSSImportRule.cpp:41: if (m_styleSheetCSSOMWrapper)
On 2014/03/12 12:24:42, Mads Ager (chromium) wrote:
> I think you still need this when !ENABLE(OILPAN).

Done.

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSRuleL...
File Source/core/css/CSSRuleList.h (right):

https://codereview.chromium.org/192473003/diff/80001/Source/core/css/CSSRuleL...
Source/core/css/CSSRuleList.h:88: // The rule owns the live list. FIXME: oilpan:
Delete this comment.
On 2014/03/12 12:24:42, Mads Ager (chromium) wrote:
> Let's just remove this comment. The fact that OwnPtrs are used should signal
> that this is owned by the rule.

Done.

> Did you look at my first round of comments? ;-)

Oops!

[Erik Corry, 2014-03-12 13:52:04]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-12 13:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/192473003/100001

[commit-bot: I haz the power, 2014-03-12 15:11:01]

Change committed as 169021