RenderStyle needs to be taken out of the RenderArena and made to inherit from RefCounted so that RefPtr and PassRefPtr work with it.
Created attachment 24399 [details] Patch
Created attachment 24403 [details] Patch
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.
Created attachment 24422 [details] Patch #3 Address Adam's comments.
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?
Created attachment 24435 [details] Patch #4
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.
(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!
Fixed in r37637.