Summary: | REGRESSION: Crash in WebCore::RenderObject::recalcMinMaxWidths() (in the counter implementation) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Lilek <dev+webkit> | ||||||||||
Component: | Layout and Rendering | Assignee: | 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
Matt Lilek
2006-11-09 13:38:31 PST
Created attachment 11447 [details]
Full crashlog
Nevermind, this was fixed in r17687 (out of date nightly). 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.
Got a patch that fixes this. Created attachment 12251 [details]
patch with a lot of other stuff in it (layout tests will be a separate patch)
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 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.
(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. Created attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)
Comment on attachment 12252 [details]
layout tests (not including css2.1 directory or any binaries for the PNGs)
r=me
Committed revision 18637. |