Bug 21647 – RenderStyle should inherit from RefCounted

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

RenderStyle needs to be taken out of the RenderArena and made to inherit from RefCounted so that RefPtr and PassRefPtr work with it.

[Dave Hyatt]

Created attachment 24399 [details]
Patch

[Dave Hyatt]

Created attachment 24403 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 24403 [details]
Patch

Your ChangeLog should at least include these two bits of information:

1) This patch makes RenderStyles not be allocated inside a RenderArena
2) (1) is OK to do because....

@@ RenderStyle* CSSStyleSelector::styleForE
11781165         m_style->setUnique();
11791166 
11801167     // Now return the style.
1181      return m_style;
 1168     return m_style.release();
11821169 }

This will set m_style to 0. The old code didn't do that. Why is that OK?

@@ RenderStyle* CSSStyleSelector::pseudoSty
13161300     if (m_fontDirty)
13171301         updateFont();
13181302     // Clean up our style object's display and text decorations (among other fixups).
1319      adjustRenderStyle(m_style, 0);
 1303     adjustRenderStyle(style(), 0);
13201304 
13211305     // Now return the style.
1322      return m_style;
 1306     return m_style.release();
13231307 }

Same question here.

Why are these two functions modifying m_style at all? Their names make it sound like they should be const member functions.

@@ inline void ElementRareData::resetComput
5251 {
5352     if (!m_computedStyle)
5453         return;
55      m_computedStyle->deref(element->document()->renderArena());
5654     m_computedStyle = 0;
5755 }

I think we prefer .clear() instead of = 0 for clearing RefPtrs. You have this in a few other places in this patch.

 1069 void Node::setRenderStyle(const PassRefPtr<RenderStyle>& s)

Why are you using a const PassRefPtr& instead of just a PassRefPtr? Ditto for all derived classes, of course.

@@ RenderStyle* AnimationController::update
234234     if ((!oldStyle || (!oldStyle->animations() && !oldStyle->transitions())) && (!newStyle->animations() && !newStyle->transitions()))
235235         return newStyle;
236236 
237      RenderStyle* blendedStyle = newStyle;
 237     RefPtr<RenderStyle> blendedStyle = newStyle;

This will ref/deref where we didn't used to before. Why is this needed?

183183     virtual void animate(CompositeAnimation*, RenderObject*, const RenderStyle* currentStyle, 
184                           const RenderStyle* targetStyle, RenderStyle*& animatedStyle) { }
 184                          const RenderStyle* targetStyle, RefPtr<RenderStyle>& animatedStyle) { }

Is animatedStyle an out parameter? Why not just return a PassRefPtr?

 229 PassRefPtr<RenderStyle> CompositeAnimationPrivate::animate(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
230230 {
231      RenderStyle* resultStyle = 0;
 231     RefPtr<RenderStyle> resultStyle;
232232 
233233     // Update animations first so we can see if any transitions are overridden
234234     updateKeyframeAnimations(renderer, currentStyle, targetStyle);

Somewhere you should have "return resultStyle.release();" But I don't see that in this patch.

 37 ImplicitAnimation::ImplicitAnimation(const Animation* transition, int animatingProperty, RenderObject* renderer, CompositeAnimation* compAnim, RenderStyle* fromStyle)
3838     : AnimationBase(transition, renderer, compAnim)
3939     , m_transitionProperty(transition->property())
4040     , m_animatingProperty(animatingProperty)
4141     , m_overridden(false)
42      , m_fromStyle(0)
43      , m_toStyle(0)
4442 {
4543     ASSERT(animatingProperty != cAnimateAll);
46      if (fromStyle) {
 44     if (fromStyle)
4745         m_fromStyle = fromStyle;
48          const_cast<RenderStyle*>(m_fromStyle)->ref();
49      }

You can just initialize m_fromStyle in the initializer list. The if isn't helping you at all.

 78         animatedStyle = RenderStyle::clone(targetStyle);

Maybe we should have a non-static copy() member function on RenderStyle?

 41 KeyframeAnimation::KeyframeAnimation(const Animation* animation, RenderObject* renderer, int index, CompositeAnimation* compAnim, RenderStyle* unanimatedStyle)
4242     : AnimationBase(animation, renderer, compAnim)
4343     , m_keyframes(renderer, animation->name())
4444     , m_index(index)
45      , m_unanimatedStyle(0)
4645 {
47      if (unanimatedStyle) {
48          const_cast<RenderStyle*>(unanimatedStyle)->ref();
 46     if (unanimatedStyle)
4947         m_unanimatedStyle = unanimatedStyle;
50      }

Again, you can just initialize in the initializer list. The if isn't helping.

@@ void InlineTextBox::paint(RenderObject::
376376             selectionFillColor = foreground;
377377         }
378378 
379          if (RenderStyle* pseudoStyle = object()->getPseudoStyle(RenderStyle::SELECTION)) {
 379         RefPtr<RenderStyle> pseudoStyle = object()->getPseudoStyle(RenderStyle::SELECTION);
 380         if (pseudoStyle) {

You can keep the declaration/assignment inside the if condition.

@@ void RenderContainer::addChild(RenderObj
113113             table = static_cast<RenderTable*>(afterChild);
114114         else {
115115             table = new (renderArena()) RenderTable(document() /* is anonymous */);
116              RenderStyle *newStyle = new (renderArena()) RenderStyle;
 116             RefPtr<RenderStyle> newStyle = RenderStyle::create();
117117             newStyle->inheritFrom(style());
118118             newStyle->setDisplay(TABLE);
119119             table->setStyle(newStyle);

Should you be passing newStyle.release() to setStyle? There are a number of places in this patch where you don't do this -- maybe you should do it everywhere?

 137 PassRefPtr<RenderStyle> RenderFileUploadControl::createButtonStyle(const RenderStyle* parentStyle) const
138138 {
139      RenderStyle* style = getPseudoStyle(RenderStyle::FILE_UPLOAD_BUTTON);
 139     RefPtr<RenderStyle> style = getPseudoStyle(RenderStyle::FILE_UPLOAD_BUTTON);

At some point you should be returning style.release(), but I don't see that in this patch.

 69     static RenderFlow* createAnonymousFlow(Document*, const PassRefPtr<RenderStyle>&);

Why a const PassRefPtr&?

@@ Color RenderObject::selectionForegroundC
20562056     if (style()->userSelect() == SELECT_NONE)
20572057         return color;
20582058 
2059      if (RenderStyle* pseudoStyle = getPseudoStyle(RenderStyle::SELECTION)) {
 2059     if (RefPtr<RenderStyle> pseudoStyle = getPseudoStyle(RenderStyle::SELECTION)) {

I don't understand why you had to add a RefPtr here. You didn't remove any calls to deref, so getPseudoStyle must not have been handing off ownership to the caller. So why use a RefPtr? There are other places in this patch where this happens, too.

[Dave Hyatt]

Created attachment 24422 [details]
Patch #3

Address Adam's comments.

[Adam Roben (:aroben)]

Comment on attachment 24422 [details]
Patch #3

@@ inline void ElementRareData::resetComput
5251 {
5352     if (!m_computedStyle)
5453         return;
55      m_computedStyle->deref(element->document()->renderArena());
56      m_computedStyle = 0;
 54     m_computedStyle.clear();
5755 }

The if here isn't needed anymore.

152  void HTMLOptGroupElement::setRenderStyle(RenderStyle* newStyle)
 146 void HTMLOptGroupElement::setRenderStyle(PassRefPtr<RenderStyle> newStyle)
153147 {
154      RenderStyle* oldStyle = m_style;
155148     m_style = newStyle;
156      if (newStyle)
157          newStyle->ref();
158      if (oldStyle)
159          oldStyle->deref(document()->renderArena());
160149 }
161150     
162151 RenderStyle* HTMLOptGroupElement::nonRendererRenderStyle() const 
163152 { 
164      return m_style; 
 153     return m_style.get(); 
165154 }

You could even move these into the header now.

Ditto for HTMLOptionElement.

183183     virtual void animate(CompositeAnimation*, RenderObject*, const RenderStyle* currentStyle, 
184                           const RenderStyle* targetStyle, RenderStyle*& animatedStyle) { }
 184                          const RenderStyle* targetStyle, RefPtr<RenderStyle>& animatedStyle) { }

Why not just have this function return a PassRefPtr, rather than using an out parameter?

@@ RenderStyle* AnimationController::update
234234     if ((!oldStyle || (!oldStyle->animations() && !oldStyle->transitions())) && (!newStyle->animations() && !newStyle->transitions()))
235235         return newStyle;
236236 
237      RenderStyle* blendedStyle = newStyle;
 237     RefPtr<RenderStyle> blendedStyle = newStyle;

I think you can remove this line of code and just declare blendedStyle the first time it's used a few lines later.

page/animation/AnimationController.h

2929 #ifndef AnimationController_h
3030 #define AnimationController_h
3131 
 32 #include <wtf/PassRefPtr.h>
 33 

I think you can use wtf/Forward.h here instead.

 229 PassRefPtr<RenderStyle> CompositeAnimationPrivate::animate(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
230230 {
231      RenderStyle* resultStyle = 0;
 231     RefPtr<RenderStyle> resultStyle;

Seems like you're missing a .release() in this function.

@@ void RenderContainer::updateBeforeAfterC
324324                     genChild->setStyle(pseudoElementStyle);
325325                 else if (genChild->isImage()) {
326326                     // Images get an empty style that inherits from the pseudo.
327                      RenderStyle* style = new (renderArena()) RenderStyle;
 327                     RefPtr<RenderStyle> style = RenderStyle::create();
328328                     style->inheritFrom(pseudoElementStyle);
329329                     genChild->setStyle(style);
330330                 } else

@@ void RenderContainer::updateBeforeAfterC
353353                 break;
354354             case CONTENT_OBJECT: {
355355                 RenderImageGeneratedContent* image = new (renderArena()) RenderImageGeneratedContent(document()); // anonymous object
356                  RenderStyle* style = new (renderArena()) RenderStyle;
 356                 RefPtr<RenderStyle> style = RenderStyle::create();
357357                 style->inheritFrom(pseudoElementStyle);
358358                 image->setStyle(style);
359359                 if (StyleImage* styleImage = content->m_content.m_image)

Missing .release()s in both of these cases.

 2165 void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
21662166 {
21672167     if (m_style == style)
21682168         return;
21692169 
21702170     RenderStyle::Diff diff = RenderStyle::Equal;
21712171     if (m_style)
2172          diff = m_style->diff(style);
 2172         diff = m_style->diff(style.get());
21732173 
21742174     // If we have no layer(), just treat a RepaintLayer hint as a normal Repaint.
21752175     if (diff == RenderStyle::RepaintLayer && !hasLayer())
21762176         diff = RenderStyle::Repaint;
21772177 
2178      styleWillChange(diff, style);
 2178     styleWillChange(diff, style.get());
21792179     
2180      RenderStyle* oldStyle = m_style;
2181      m_style = const_cast<RenderStyle*>(style);
2182      if (m_style)
2183          m_style->ref();
 2180     RefPtr<RenderStyle> oldStyle = m_style;
 2181     m_style = style;

You can make this a little more efficient by doing:

RefPtr<RenderStyle> oldStyle = m_style.release();

 2192 void RenderObject::setStyleInternal(PassRefPtr<RenderStyle> style)
21992193 {
22002194     if (m_style == style)
22012195         return;
2202      if (m_style)
2203          m_style->deref(renderArena());
22042196     m_style = style;
2205      if (m_style)
2206          m_style->ref();
22072197 }

The if here isn't needed anymore.

 379     RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
 380     PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;

I think it's worth adding comments about how one should choose which of these two functions to call.

 38 void RenderSVGBlock::setStyle(PassRefPtr<RenderStyle> style) 
3939 {
40      const RenderStyle* useStyle = style;
 40     RefPtr<RenderStyle> useStyle = style;
4141 
4242     // SVG text layout code expects us to be a block-level style element.   
4343     if (useStyle->display() == NONE)
4444         setChildrenInline(false);
4545     else if (useStyle->isDisplayInlineType()) {
46          RenderStyle* newStyle = new (renderArena()) RenderStyle();
47          newStyle->inheritFrom(style);
 46         RefPtr<RenderStyle> newStyle = RenderStyle::create();
 47         newStyle->inheritFrom(useStyle.get());
4848         newStyle->setDisplay(BLOCK);
4949         useStyle = newStyle;
5050     }

You can do:

useStyle = newStyle.release();

 177 PassRefPtr<RenderStyle> RenderSlider::createThumbStyle(const RenderStyle* parentStyle, const RenderStyle* oldStyle)

I think you're missing a .release() in this function.

 36 inline RenderStyle* defaultStyle()
3937 {
40      return renderArena->allocate(sz);
 38     static RefPtr<RenderStyle> s_defaultStyle;
 39     if (!s_defaultStyle)
 40         s_defaultStyle = RenderStyle::createDefaultStyle();
 41     return s_defaultStyle.get();
4142 }

It would be better just to use a bare (leaked) pointer here so that we don't have to call deref() when the process is exiting. You also don't need the if -- you can just initialize on the same line you declare it on.

142121 RenderStyle::RenderStyle(const RenderStyle& o)
143      : inherited_flags(o.inherited_flags)
 122     : RefCounted<RenderStyle>()

The RefCounted() shouldn't be necessary.

Have you tried run-webkit-tests --leaks?

[Dave Hyatt]

Created attachment 24435 [details]
Patch #4

[Adam Roben (:aroben)]

Comment on attachment 24435 [details]
Patch #4

 379     // The pseudo element style can be cached or uncached.  Use the cached method if the pseudo element doesn't respect
 380     // any pseudo classes (and therefore has no concept of changing state).
 381     RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;
 382     PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId, RenderStyle* parentStyle = 0) const;

Is it possible to assert that people are calling the right function?

 36 inline RenderStyle* defaultStyle()
3937 {
 38     RenderStyle* s_defaultStyle = RenderStyle::createDefaultStyle().releaseRef();
 39     return s_defaultStyle;
4140 }

I think you meant to declare s_defaultStyle static.

142119 RenderStyle::RenderStyle(const RenderStyle& o)
143      : inherited_flags(o.inherited_flags)
 120     : RefCounted<RenderStyle>()
 121     , inherited_flags(o.inherited_flags)

The RefCounted() isn't necessary here.

Were we leaking RenderStyle::pseudoStyle before? I don't see any existing calls to deref() it.

r=me if you fix the defaultStyle() bug.

[Dave Hyatt]

(In reply to comment #7)
> (From update of attachment 24435 [details] [edit])
>  379     // The pseudo element style can be cached or uncached.  Use the cached
> method if the pseudo element doesn't respect
>  380     // any pseudo classes (and therefore has no concept of changing
> state).
>  381     RenderStyle* getCachedPseudoStyle(RenderStyle::PseudoId, RenderStyle*
> parentStyle = 0) const;
>  382     PassRefPtr<RenderStyle> getUncachedPseudoStyle(RenderStyle::PseudoId,
> RenderStyle* parentStyle = 0) const;
> 
> Is it possible to assert that people are calling the right function?
> 

I didn't want to make any assumptions about particular pseudo elements for now.

>  36 inline RenderStyle* defaultStyle()
> 3937 {
>  38     RenderStyle* s_defaultStyle =
> RenderStyle::createDefaultStyle().releaseRef();
>  39     return s_defaultStyle;
> 4140 }
> 
> I think you meant to declare s_defaultStyle static.
> 

Indeed.

> 142119 RenderStyle::RenderStyle(const RenderStyle& o)
> 143      : inherited_flags(o.inherited_flags)
>  120     : RefCounted<RenderStyle>()
>  121     , inherited_flags(o.inherited_flags)
> 
> The RefCounted() isn't necessary here.
> 

Yes it is.

> Were we leaking RenderStyle::pseudoStyle before? I don't see any existing calls
> to deref() it.
> 
> r=me if you fix the defaultStyle() bug.
> 

Thanks for the comments!

[Dave Hyatt]

Fixed in r37637.