Bug 11557 - REGRESSION: Crash in WebCore::RenderObject::recalcMinMaxWidths() (in the counter implementation)
Summary: REGRESSION: Crash in WebCore::RenderObject::recalcMinMaxWidths() (in the coun...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://www.css3.info/modules
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2006-11-09 13:38 PST by Matt Lilek
Modified: 2007-01-05 17:53 PST (History)
2 users (show)

See Also:


Attachments
Full crashlog (19.49 KB, text/plain)
2006-11-09 13:39 PST, Matt Lilek
no flags Details
Reduction (453 bytes, text/html)
2006-11-12 02:50 PST, mitz
no flags Details
patch with a lot of other stuff in it (layout tests will be a separate patch) (260.39 KB, patch)
2007-01-05 16:33 PST, Darin Adler
hyatt: review+
Details | Formatted Diff | Diff
layout tests (not including css2.1 directory or any binaries for the PNGs) (328.33 KB, patch)
2007-01-05 16:57 PST, Darin Adler
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.