Move StyleSheet to the oilpan heap using transition types.

Move StyleSheet to the oilpan heap using transition types.

This makes StyleSheets RefCountedGarbageCollected. Next step is
removing RefPtrs and raw pointers and dealing with finalization
order issues.

R=erik.corry@gmail.com, haraken@chromium.org, wibling@chromium.org
BUG=

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

[Mads Ager (chromium), 2014-03-05 11:16:36]

This is a reland of a previous change that was reverted. As expected the issue
was an ordering assumption in the iteration of clients for a StyleSheetContents.
The code for that was pretty funky anyway, so I reworked that to protect all the
elements while iterating instead of protecting them one at a time.

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:404: // Avoid |CSSSStyleSheet| and
|ownerNode| being deleted by scripts that run via
The protection during iteration has changed here. The protection that was going
on here was shaky. It turned out that the last sheet in all layout tests was the
only one that had an owner and therefore the old protection worked. That is
pretty funky. Instead of protecting each element one at a time during the
iteration, we protect them all up front and let them die after the iteration.
That seems saner in any case in the current system and can be removed once we
ship with oilpan.

[Erik Corry, 2014-03-05 11:33:28]

lgtm

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:410:
WillBeHeapHashSet<RefPtrWillBeMember<CSSStyleSheet> > protectedClients;
I think this can be a vector, then you don't have to rehash and it makes the
iteration a little simpler.

Also you need to revisit the comment.

[Mads Ager (chromium), 2014-03-05 11:36:04]

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/187313005/diff/40002/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:410:
WillBeHeapHashSet<RefPtrWillBeMember<CSSStyleSheet> > protectedClients;
On 2014/03/05 11:33:29, Erik Corry wrote:
> I think this can be a vector, then you don't have to rehash and it makes the
> iteration a little simpler.
> 
> Also you need to revisit the comment.

Good idea, that will simplify. I have replaced it with a Vector with inline
capacity 5 and I then iterate that. Thanks.

[wibling-chromium, 2014-03-05 11:36:48]

lgtm

[Mads Ager (chromium), 2014-03-05 12:23:01]

There is a finalization issue here that I missed. The StyleEngine used to die
together with the Document (OwnPtr) and in its destructor it is assumed that the
document is still accessible because the CSSStyleSheet accesses it.

For this change I will just revert the StyleEngine change. There is no reason to
move that to the heap for this change.

[Mads Ager (chromium), 2014-03-05 12:43:46]

StyleEngine change reverted. Haraken, this is ready for your review when you get
the time. :)

[haraken, 2014-03-05 14:21:21]

LGTM with comments.

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.h:87: void clearOwnerRule() { m_ownerRule =
nullptr; }

Can we add #if !ENABLE_OILPAN to this method?

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/MediaList.h
File Source/core/css/MediaList.h (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/MediaLis...
Source/core/css/MediaList.h:96: void clearParentRule() { ASSERT(m_parentRule);
m_parentRule = nullptr; }

Shall we add #if !ENABLE_OILPAN? The #if will be helpful to find methods we
should remove when moving objects to real types.

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:405: //
ScriptableDocumentParser::executeScriptsWaitingForResources(). Also protect

FYI, I'm planning to make a change that moves
ScriptableDocumentParser::executeScriptsWaitingForResources() to the next event
loop (to fix another protection bug). It will enable us to remove the
protection.

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

https://codereview.chromium.org/187313005/diff/90001/Source/core/xml/XSLStyle...
Source/core/xml/XSLStyleSheet.h:122: XSLStyleSheet* m_parentStyleSheet;

Shouldn't we use RawPtrWillBeMember<XSLStyleSheet> ?

[Mads Ager (chromium), 2014-03-05 14:51:15]

Thanks Haraken.

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.h:87: void clearOwnerRule() { m_ownerRule =
nullptr; }
On 2014/03/05 14:21:22, haraken wrote:
> 
> Can we add #if !ENABLE_OILPAN to this method?

No, it is called from a destructor which is no longer needed but it is also
called when setStyleSheet is called on the owner.

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/MediaList.h
File Source/core/css/MediaList.h (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/MediaLis...
Source/core/css/MediaList.h:96: void clearParentRule() { ASSERT(m_parentRule);
m_parentRule = nullptr; }
On 2014/03/05 14:21:22, haraken wrote:
> 
> Shall we add #if !ENABLE_OILPAN? The #if will be helpful to find methods we
> should remove when moving objects to real types.

I'm a little worried about the amount of #if !ENABLE(OILPAN) that we will get.
But it is a good idea to make it clear that this is dead code. 

Done.

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/StyleShe...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/187313005/diff/90001/Source/core/css/StyleShe...
Source/core/css/StyleSheetContents.cpp:405: //
ScriptableDocumentParser::executeScriptsWaitingForResources(). Also protect
On 2014/03/05 14:21:22, haraken wrote:
> 
> FYI, I'm planning to make a change that moves
> ScriptableDocumentParser::executeScriptsWaitingForResources() to the next
event
> loop (to fix another protection bug). It will enable us to remove the
> protection.

The protection here is not only because of executeScriptsWaitingForResources. It
is also because the protectedClients[i]->sheetLoaded() call on line 422 can lead
to CSSStyleSheets reaching ref count zero. That was the problem when I landed
this change the first time.

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

https://codereview.chromium.org/187313005/diff/90001/Source/core/xml/XSLStyle...
Source/core/xml/XSLStyleSheet.h:122: XSLStyleSheet* m_parentStyleSheet;
On 2014/03/05 14:21:22, haraken wrote:
> 
> Shouldn't we use RawPtrWillBeMember<XSLStyleSheet> ?

Yes, in one of the next changes. When I remove the RefPtrs we will need to get
these right.

[Mads Ager (chromium), 2014-03-05 14:53:22]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-05 14:54:10]

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

[commit-bot: I haz the power, 2014-03-05 20:59:01]

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

[commit-bot: I haz the power, 2014-03-06 02:04:58]

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

[commit-bot: I haz the power, 2014-03-06 02:04:58]

Try jobs failed on following builders: win_blink_rel

[Mads Ager (chromium), 2014-03-06 06:35:16]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:35:28]

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

[commit-bot: I haz the power, 2014-03-06 07:51:43]

Change committed as 168583