REGRESSION: Crash in WebCore::RenderObject::recalcMinMaxWidths() (in the counter implementation)

[Matt Lilek]

Above URL causes a crash in WebCore::RenderObject::recalcMinMaxWidths().

[Matt Lilek]

Created attachment 11447 [details]
Full crashlog

[Matt Lilek]

Nevermind, this was fixed in r17687 (out of date nightly).

[Matt Lilek]

This is crashing for me again with the exact same back trace with the 17739 nightly.

Created attachment 11493 [details]
Reduction

Here's what I think the problem is: RenderCounter's m_counter is initialized with a pointer to CounterData that comes from the parent's 'before' pseudo RenderStyle. When the parent's style changes, that RenderStyle and the CounterData are deleted, and new ones are created (if there's still 'before' content with a counter), but updatePseudoChildForObject() doesn't update the RenderCounter's m_counter pointer. Eventually RenderCounter::calcMinMaxWidth() is called and tries to use the deleted CounterData.

On a release build, this doesn't crash right away, but if you play with the text size in Safari (Command + and Command -) you might see the counter change randomly from a number to a bullet.

[Darin Adler]

Got a patch that fixes this.

[Darin Adler]

Created attachment 12251 [details]
patch with a lot of other stuff in it (layout tests will be a separate patch)

[Darin Adler]

I'll need to update the change log to reflect the fact that this bug number is fixed by the patch.

The CSS 2.1 tests exercise counters enough to show the same crash that the reduction does.

[Dave Hyatt]

Comment on attachment 12251 [details]
patch with a lot of other stuff in it (layout tests will be a separate patch)

RenderBlock derives from RenderBox, so for this code in RenderBlock.h....

+        if (RenderBox::marginTop() >= 0) {
+            m_maxTopPosMargin = RenderBox::marginTop();

Why "RenderBox::"?  RenderBox is a base class so no need to qualify that function call explicitly.

I don't understand this change:

-            static_cast<RenderText*>(newChild)->setText(textToTransform.get(), true);
+            static_cast<RenderText*>(newChild)->setText(textToTransform.release(), true);

r=me, fix RenderBlock.h though, I'll assume you know what you're doing with the text transform thing... I've never seen .release() before though.

[Darin Adler]

(In reply to comment #8)
> RenderBlock derives from RenderBox, so for this code in RenderBlock.h....
> 
> +        if (RenderBox::marginTop() >= 0) {
> +            m_maxTopPosMargin = RenderBox::marginTop();
> 
> Why "RenderBox::"?  RenderBox is a base class so no need to qualify that
> function call explicitly.

Good point.

I was thinking that since marginTop was a virtual function I wanted to be sure to not get any derived version, but I see now that marginTop is only in RenderObject and RenderBox.

It's annoying that we have to pay for a virtual function call, though. I might add fastMarginTop() like we have fastFirstChild().

> I don't understand this change:
> 
> -            static_cast<RenderText*>(newChild)->setText(textToTransform.get(),
> true);
> +           
> static_cast<RenderText*>(newChild)->setText(textToTransform.release(), true);

The release trick turns a RefPtr into a PassRefPtr, setting the RefPtr to 0. It saves you one round of reference count churn, because setText doesn't have to ref the passed-in text.

I'm supposed to write up a little document explaining this. Maybe I'll do it this weekend.

[Darin Adler]

Created attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)

[Dave Hyatt]

Comment on attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)

r=me

[Darin Adler]

Committed revision 18637.