Bug 11557

Summary: REGRESSION: Crash in WebCore::RenderObject::recalcMinMaxWidths() (in the counter implementation)
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: Layout and RenderingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, mitz
Priority: P1 Keywords: HasReduction, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.css3.info/modules
Attachments:
Description Flags
Full crashlog
none
Reduction
none
patch with a lot of other stuff in it (layout tests will be a separate patch)
hyatt: review+
layout tests (not including css2.1 directory or any binaries for the PNGs) hyatt: review+

Description Matt Lilek 2006-11-09 13:38:31 PST
Above URL causes a crash in WebCore::RenderObject::recalcMinMaxWidths().
Comment 1 Matt Lilek 2006-11-09 13:39:39 PST
Created attachment 11447 [details]
Full crashlog
Comment 2 Matt Lilek 2006-11-09 15:59:57 PST
Nevermind, this was fixed in r17687 (out of date nightly).
Comment 3 Matt Lilek 2006-11-11 20:22:37 PST
This is crashing for me again with the exact same back trace with the 17739 nightly.
Comment 4 mitz 2006-11-12 02:50:08 PST
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.
Comment 5 Darin Adler 2006-12-30 00:58:59 PST
Got a patch that fixes this.
Comment 6 Darin Adler 2007-01-05 16:33:54 PST
Created attachment 12251 [details]
patch with a lot of other stuff in it (layout tests will be a separate patch)
Comment 7 Darin Adler 2007-01-05 16:34:43 PST
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.
Comment 8 Dave Hyatt 2007-01-05 16:45:22 PST
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.
Comment 9 Darin Adler 2007-01-05 16:50:59 PST
(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.
Comment 10 Darin Adler 2007-01-05 16:57:12 PST
Created attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)
Comment 11 Dave Hyatt 2007-01-05 17:09:44 PST
Comment on attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)

r=me
Comment 12 Darin Adler 2007-01-05 17:53:58 PST
Committed revision 18637.