Change various helper classes to transition types to get CSSValue entirely onto the gc heap.

Change various helper classes to transition types to get CSSValue entirely onto the gc heap.

Specifically MediaQuery, MediaQueryExp, MediaQuerySet, MediaQueryResult, and MediaList. Also added a persistent in  CSSStyleSheet and MediaQueryList. Finally changed FontFace to RefCountedGarbageCollected when oilpan is enabled. FontFace require a bit more separate work to move entirely so leaving that for a separate change.

The reason for the test breakage was that I had overlooked a MediaQueryExp that was a part object in MediaQueryResult.  

Finally fixed an incorrect persistent found by Ian's oilpan plugin, yay:)

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
NOTRY=true

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

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

[tkent, 2014-02-21 00:00:36]

https://codereview.chromium.org/170283019/diff/1/Source/core/css/CSSGrammar.y
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/CSSGrammar.y...
Source/core/css/CSSGrammar.y:87:
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> >* mediaQueryExpList;
How is this union allocated?  On stack?

[tkent, 2014-02-21 00:13:14]

lgtm

https://codereview.chromium.org/170283019/diff/1/Source/core/css/CSSGrammar.y
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/CSSGrammar.y...
Source/core/css/CSSGrammar.y:87:
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> >* mediaQueryExpList;
On 2014/02/21 00:00:36, tkent wrote:
> How is this union allocated?  On stack?

Ah, this is a pointer. So allocation type doesn't matter.

[haraken, 2014-02-21 01:02:36]

LGTM with comments.

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cp...
Source/core/css/MediaList.cpp:166: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).

OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >

Can we add a dummy trace for Vector<OwnPtr> and avoid adding ENABLE(OILPAN) ?

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

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.h#...
Source/core/css/MediaList.h:112: CSSRule* m_parentRule;

The lifetime of these raw pointers are a bit complicated, and it's not obvious
that they don't cause lifetime issue when MediaList outlives CSSStyleSheet or
CSSRule. Would you double-check it and add a comment?

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQuery.cpp
File Source/core/css/MediaQuery.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQuery.c...
Source/core/css/MediaQuery.cpp:133: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).

Ditto.

OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQueryEx...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQueryEx...
Source/core/css/MediaQueryExp.cpp:200: MediaQueryExp::MediaQueryExp(const
MediaQueryExp& other)

I'm just curious: Why do you need to add the copy constructor?

https://codereview.chromium.org/170283019/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser.h:303:
OwnPtrWillBePersistent<WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> > >
m_floatingMediaQueryExpList;

Can you use WillBePersistentHeapVector<OwnPtrWillBeMember<>> ?

[wibling-chromium, 2014-02-21 08:43:07]

Thanks for the reviews!

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cp...
Source/core/css/MediaList.cpp:166: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).
On 2014/02/21 01:02:36, haraken wrote:
> 
> OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >
> 
> Can we add a dummy trace for Vector<OwnPtr> and avoid adding ENABLE(OILPAN) ?

We talked a bit about this locally. We could fix the OffHeapCollectionTrait to
call the visitor->trace instead of calling the tracetrait. That would make it
work AFAICT. I will do a bit of experimenting locally with the oilpan branch to
see if that fixes the issue.
This would basically allow us to support tracing of Vector's of ownptrs
independent of whether oilpan flag is enabled.

If I instead add a dummy trace trait it would mean we could not support tracing
vector's of ownptrs even in the case where e.g. the modules wanted to do that.

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

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.h#...
Source/core/css/MediaList.h:112: CSSRule* m_parentRule;
On 2014/02/21 01:02:36, haraken wrote:
> 
> The lifetime of these raw pointers are a bit complicated, and it's not obvious
> that they don't cause lifetime issue when MediaList outlives CSSStyleSheet or
> CSSRule. Would you double-check it and add a comment?

Yes, I will investigate it a bit further.

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQuery.cpp
File Source/core/css/MediaQuery.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQuery.c...
Source/core/css/MediaQuery.cpp:133: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).
On 2014/02/21 01:02:36, haraken wrote:
> 
> Ditto.
> 
> OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >

Same.

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQueryEx...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaQueryEx...
Source/core/css/MediaQueryExp.cpp:200: MediaQueryExp::MediaQueryExp(const
MediaQueryExp& other)
On 2014/02/21 01:02:36, haraken wrote:
> 
> I'm just curious: Why do you need to add the copy constructor?

Because of the copy method in the .h file. It call MediaQueryExp(*this) and the
implicit/default copy constructor is deleted in the GarbageCollected parent.

https://codereview.chromium.org/170283019/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser.h:303:
OwnPtrWillBePersistent<WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> > >
m_floatingMediaQueryExpList;
On 2014/02/21 01:02:36, haraken wrote:
> 
> Can you use WillBePersistentHeapVector<OwnPtrWillBeMember<>> ?

Yes, thanks for catching this.

[wibling-chromium, 2014-02-25 12:07:28]

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cp...
Source/core/css/MediaList.cpp:166: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).
On 2014/02/21 08:43:08, wibling-chromium wrote:
> On 2014/02/21 01:02:36, haraken wrote:
> > 
> > OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >
> > 
> > Can we add a dummy trace for Vector<OwnPtr> and avoid adding ENABLE(OILPAN)
?
> 
> We talked a bit about this locally. We could fix the OffHeapCollectionTrait to
> call the visitor->trace instead of calling the tracetrait. That would make it
> work AFAICT. I will do a bit of experimenting locally with the oilpan branch
to
> see if that fixes the issue.
> This would basically allow us to support tracing of Vector's of ownptrs
> independent of whether oilpan flag is enabled.
> 
> If I instead add a dummy trace trait it would mean we could not support
tracing
> vector's of ownptrs even in the case where e.g. the modules wanted to do that.

I tried this above and both when calling visitor->trace in the
OffHeapCollectionTrait and when defining a dummy TraceTrait for the OwnPtr I got
into problems with the DefaultAllocatorDummyVisitor class defined in WTF and
that it was not assignable to the WebCore::Visitor class. I will leave the
ifdef's in for now and we can see if we come up with something clever in the
future.
This should go away together with the transition types.

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

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.h#...
Source/core/css/MediaList.h:112: CSSRule* m_parentRule;
On 2014/02/21 01:02:36, haraken wrote:
> 
> The lifetime of these raw pointers are a bit complicated, and it's not obvious
> that they don't cause lifetime issue when MediaList outlives CSSStyleSheet or
> CSSRule. Would you double-check it and add a comment?

From what I can tell there are three callers to MediaList::create.

CSSStyleSheet.cpp:374
CSSMediaRule.cpp:66
CSSImportRule.cpp:55

The first sets the m_parentStyleSheet raw pointer and clears it via
clearParentStyleSheet when it is destructed. The two latter sets m_parentRule
and clears it via clearParentRule when they are destructed. AFAICT this is okay
and we should not have a dangling raw pointer to either in the case MediaList
outlives them.

[haraken, 2014-02-25 12:09:57]

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.cp...
Source/core/css/MediaList.cpp:166: // We don't support tracing of vectors of
OwnPtrs (ie. OwnPtr<Vector<OwnPtr<MediaQuery> > >).
On 2014/02/25 12:07:28, wibling-chromium wrote:
> On 2014/02/21 08:43:08, wibling-chromium wrote:
> > On 2014/02/21 01:02:36, haraken wrote:
> > > 
> > > OwnPtr<Vector<OwnPtr<MediaQuery> > > => Vector<OwnPtr<MediaQuery> >
> > > 
> > > Can we add a dummy trace for Vector<OwnPtr> and avoid adding
ENABLE(OILPAN)
> ?
> > 
> > We talked a bit about this locally. We could fix the OffHeapCollectionTrait
to
> > call the visitor->trace instead of calling the tracetrait. That would make
it
> > work AFAICT. I will do a bit of experimenting locally with the oilpan branch
> to
> > see if that fixes the issue.
> > This would basically allow us to support tracing of Vector's of ownptrs
> > independent of whether oilpan flag is enabled.
> > 
> > If I instead add a dummy trace trait it would mean we could not support
> tracing
> > vector's of ownptrs even in the case where e.g. the modules wanted to do
that.
> 
> I tried this above and both when calling visitor->trace in the
> OffHeapCollectionTrait and when defining a dummy TraceTrait for the OwnPtr I
got
> into problems with the DefaultAllocatorDummyVisitor class defined in WTF and
> that it was not assignable to the WebCore::Visitor class. I will leave the
> ifdef's in for now and we can see if we come up with something clever in the
> future.
> This should go away together with the transition types.

Makes sense. Having #if in a couple of places in a transition period wouldn't be
a big deal.

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

https://codereview.chromium.org/170283019/diff/1/Source/core/css/MediaList.h#...
Source/core/css/MediaList.h:112: CSSRule* m_parentRule;
On 2014/02/25 12:07:28, wibling-chromium wrote:
> On 2014/02/21 01:02:36, haraken wrote:
> > 
> > The lifetime of these raw pointers are a bit complicated, and it's not
obvious
> > that they don't cause lifetime issue when MediaList outlives CSSStyleSheet
or
> > CSSRule. Would you double-check it and add a comment?
> 
> From what I can tell there are three callers to MediaList::create.
> 
> CSSStyleSheet.cpp:374
> CSSMediaRule.cpp:66
> CSSImportRule.cpp:55
> 
> The first sets the m_parentStyleSheet raw pointer and clears it via
> clearParentStyleSheet when it is destructed. The two latter sets m_parentRule
> and clears it via clearParentRule when they are destructed. AFAICT this is
okay
> and we should not have a dangling raw pointer to either in the case MediaList
> outlives them.

Got it, thanks!

[wibling-chromium, 2014-02-25 12:41:06]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-02-25 12:41:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/170283019/200001

[commit-bot: I haz the power, 2014-02-25 13:33:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/170283019/200001

[commit-bot: I haz the power, 2014-02-25 16:21:20]

Change committed as 167798

[haraken, 2014-02-26 05:30:20]

On 2014/02/25 16:21:20, I haz the power (commit-bot) wrote:
> Change committed as 167798

hmm, let me revert this CL, since it caused a ton of crashes in oilpan bots:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

[wibling-chromium, 2014-02-26 07:41:15]

On 2014/02/26 05:30:20, haraken wrote:
> On 2014/02/25 16:21:20, I haz the power (commit-bot) wrote:
> > Change committed as 167798
> 
> hmm, let me revert this CL, since it caused a ton of crashes in oilpan bots:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Linux%2520Oil...

Thanks for reverting. I should have rerun tests after sync'ing it back up:(

[wibling-chromium, 2014-03-03 10:27:43]

PTAL

[Mads Ager (chromium), 2014-03-03 10:38:44]

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSImpo...
File Source/core/css/CSSImportRule.cpp (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSImpo...
Source/core/css/CSSImportRule.cpp:44: // Oilpan automatically handles cycles and
doesn't want destructors to touch potentially
Maybe the comment should state that the MediaList and its parent CSSImport rules
died together with oilpan and therefore the clearing is not necessary?

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSMedi...
File Source/core/css/CSSMediaRule.cpp (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSMedi...
Source/core/css/CSSMediaRule.cpp:38: if (m_mediaCSSOMWrapper)
This is unsafe. They are both in the heap now. Do they have a strong pointer
relationship now so they live and die together? In that case you should just
ifdef this out for the oilpan build.

[wibling-chromium, 2014-03-03 10:45:08]

Thanks for the review. I will upload a new diff.

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSImpo...
File Source/core/css/CSSImportRule.cpp (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSImpo...
Source/core/css/CSSImportRule.cpp:44: // Oilpan automatically handles cycles and
doesn't want destructors to touch potentially
On 2014/03/03 10:38:45, Mads Ager (chromium) wrote:
> Maybe the comment should state that the MediaList and its parent CSSImport
rules
> died together with oilpan and therefore the clearing is not necessary?

Done.

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSMedi...
File Source/core/css/CSSMediaRule.cpp (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/CSSMedi...
Source/core/css/CSSMediaRule.cpp:38: if (m_mediaCSSOMWrapper)
On 2014/03/03 10:38:45, Mads Ager (chromium) wrote:
> This is unsafe. They are both in the heap now. Do they have a strong pointer
> relationship now so they live and die together? In that case you should just
> ifdef this out for the oilpan build.

Thanks for catching this. I have added the ifdef.

[haraken, 2014-03-03 10:51:29]

LGTM with Mads' comments addressed.

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/MediaQu...
File Source/core/css/MediaQuery.h (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/MediaQu...
Source/core/css/MediaQuery.h:41: typedef
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> > ExpressionVector;

Probably you might want to use "ExpressionHeapVector" to make the underlying
data structure clear.

https://codereview.chromium.org/170283019/diff/240001/Source/core/storage/Sto...
File Source/core/storage/StorageEvent.h (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/storage/Sto...
Source/core/storage/StorageEvent.h:82: RefPtrWillBeMember<Storage>
m_storageArea;

Event might be reverted to RefCounted<Event> soon :)

[wibling-chromium, 2014-03-03 11:21:10]

Thanks for the review!

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/MediaQu...
File Source/core/css/MediaQuery.h (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/css/MediaQu...
Source/core/css/MediaQuery.h:41: typedef
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> > ExpressionVector;
On 2014/03/03 10:51:30, haraken wrote:
> 
> Probably you might want to use "ExpressionHeapVector" to make the underlying
> data structure clear.

Done.

https://codereview.chromium.org/170283019/diff/240001/Source/core/storage/Sto...
File Source/core/storage/StorageEvent.h (right):

https://codereview.chromium.org/170283019/diff/240001/Source/core/storage/Sto...
Source/core/storage/StorageEvent.h:82: RefPtrWillBeMember<Storage>
m_storageArea;
On 2014/03/03 10:51:30, haraken wrote:
> 
> Event might be reverted to RefCounted<Event> soon :)

Yes, I know, but it seemed better to me to fix it and change it back when you
revert Event.

[Mads Ager (chromium), 2014-03-03 11:30:20]

LGTM

https://codereview.chromium.org/170283019/diff/260001/Source/core/css/CSSGram...
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/170283019/diff/260001/Source/core/css/CSSGram...
Source/core/css/CSSGrammar.y:88:
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> >* mediaQueryExpList;
As with the ruleList above, I guess there is something else in the
BisonCSSParser that is keeping these alive? Add a comment or group these
together with a common comment?

[wibling-chromium, 2014-03-03 11:40:16]

https://codereview.chromium.org/170283019/diff/260001/Source/core/css/CSSGram...
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/170283019/diff/260001/Source/core/css/CSSGram...
Source/core/css/CSSGrammar.y:88:
WillBeHeapVector<OwnPtrWillBeMember<MediaQueryExp> >* mediaQueryExpList;
On 2014/03/03 11:30:21, Mads Ager (chromium) wrote:
> As with the ruleList above, I guess there is something else in the
> BisonCSSParser that is keeping these alive? Add a comment or group these
> together with a common comment?

I will group it with the above StyleRuleBase vector. It is a bit unfortunate
that we have these, but I am not sure how to better fix it.

[wibling-chromium, 2014-03-04 09:59:29]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-04 10:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/170283019/300001

[zerny-chromium, 2014-03-05 06:38:39]

lgtm, with one fix and one suggestion.

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaLi...
File Source/core/css/MediaList.h (right):

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaLi...
Source/core/css/MediaList.h:41: class MediaQuerySet : public
RefCountedWillBeGarbageCollected<MediaQuerySet> {
+Finalized or remove the destructor.

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluator.h:41: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<MediaQueryResult> >
MediaQueryResultList;
Maybe this should be expanded at the use sites. It is a bit odd to have the
conditional persistent behind a typedef.

[wibling-chromium, 2014-03-05 08:09:41]

Thanks for the review.

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaLi...
File Source/core/css/MediaList.h (right):

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaLi...
Source/core/css/MediaList.h:41: class MediaQuerySet : public
RefCountedWillBeGarbageCollected<MediaQuerySet> {
On 2014/03/05 06:38:40, zerny-chromium wrote:
> +Finalized or remove the destructor.

As you mentioned off-line yourself this is okay as is. We only need the
destructor in the non-oilpan case where the vector needs to be destructed. Kept
the code as is.

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/170283019/diff/300001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluator.h:41: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<MediaQueryResult> >
MediaQueryResultList;
On 2014/03/05 06:38:40, zerny-chromium wrote:
> Maybe this should be expanded at the use sites. It is a bit odd to have the
> conditional persistent behind a typedef.

Agree. I will fix this is a subsequent change to not mess up the current commit.

[wibling-chromium, 2014-03-05 09:24:36]

The CQ bit was unchecked by wibling@chromium.org

[wibling-chromium, 2014-03-05 09:29:52]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-05 09:30:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/170283019/300001

[commit-bot: I haz the power, 2014-03-05 09:31:42]

Change committed as 168456