Bug 53316 – NULL pointer crash when using :empty and :first-line pseudoclass selectors together

• Component: CSS
• Status: RESOLVED FIXED

[Thomas Sepez]

The following example reproduces the crash:
<style> *:empty:first-line { background: red; } </style>
<button autofocus></button>

Crash is at:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000024
[Switching to process 7834]
0x025ccff5 in WTF::RefPtr<WebCore::StyleInheritedData>::get (this=0x24) at RefPtr.h:60
60	        T* get() const { return m_ptr; }
(gdb) where
#0  0x025ccff5 in WTF::RefPtr<WebCore::StyleInheritedData>::get (this=0x24) at RefPtr.h:60
#1  0x025f9155 in WebCore::DataRef<WebCore::StyleInheritedData>::get (this=0x24) at DataRef.h:33
#2  0x025f9169 in WebCore::DataRef<WebCore::StyleInheritedData>::operator-> (this=0x24) at DataRef.h:36
#3  0x025334a8 in WebCore::RenderStyle::lineHeight (this=0x0) at RenderStyle.h:484
#4  0x0295bea9 in WebCore::RenderStyle::computedLineHeight (this=0x0) at RenderStyle.h:487
#5  0x0293f1fc in WebCore::RenderBlock::lineHeight (this=0x9ef7e6c, firstLine=true, direction=WebCore::HorizontalLine, linePositionMode=WebCore::PositionOfInteriorLineBoxes) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:5028
#6  0x02a52eea in WebCore::RootInlineBox::lineHeight (this=0x9ee7f4c) at RootInlineBox.h:92
#7  0x029272d9 in WebCore::InlineFlowBox::computeLogicalBoxHeights (this=0x9ee7f4c, maxPositionTop=@0xb49b6264, maxPositionBottom=@0xb49b6260, maxAscent=@0xb49b625c, maxDescent=@0xb49b6258, setMaxAscent=@0xb49b626f, setMaxDescent=@0xb49b626e, strictMode=false, textBoxDataMap=@0xb49b66e8, baselineType=WebCore::AlphabeticBaseline, verticalPositionCache=@0xb49b6660) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/InlineFlowBox.cpp:460
#8  0x02a52018 in WebCore::RootInlineBox::alignBoxesInBlockDirection (this=0x9ee7f4c, heightOfBlock=0, textBoxDataMap=@0xb49b66e8, verticalPositionCache=@0xb49b6660) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RootInlineBox.cpp:242
#9  0x0296b30e in WebCore::RenderBlock::computeBlockDirectionPositionsForLine (this=0x9ef7e6c, lineBox=0x9ee7f4c, firstRun=0x9ef24bc, textBoxDataMap=@0xb49b66e8, verticalPositionCache=@0xb49b6660) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlockLineLayout.cpp:480
#10 0x0296dfe1 in WebCore::RenderBlock::layoutInlineChildren (this=0x9ef7e6c, relayoutChildren=true, repaintLogicalTop=@0xb49b6878, repaintLogicalBottom=@0xb49b6874) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlockLineLayout.cpp:756
#11 0x0294e164 in WebCore::RenderBlock::layoutBlock (this=0x9ef7e6c, relayoutChildren=true, pageLogicalHeight=0) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:1221
#12 0x0294ce18 in WebCore::RenderBlock::layout (this=0x9ef7e6c) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:1119
#13 0x0294bf03 in WebCore::RenderBlock::layoutBlockChild (this=0x9eec33c, child=0x9ef7e6c, marginInfo=@0xb49b6a2c, previousFloatLogicalBottom=@0xb49b6a54, maxFloatLogicalBottom=@0xb49b6ba0) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:1958
#14 0x0294db28 in WebCore::RenderBlock::layoutBlockChildren (this=0x9eec33c, relayoutChildren=true, maxFloatLogicalBottom=@0xb49b6ba0) at /Volumes/MacintoshHD2/c1/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:1896

...

[Thomas Sepez]

In RenderBlock::lineHeight():

   if (firstLine && document()->usesFirstLineRules()) {
        RenderStyle* s = style(firstLine);
        if (s != style())
            return s->computedLineHeight();

we're getting a NULL back from style(firstLine).

This is called once successfully, but the second call returns 0, because eventually RenderObject::getUncachedPseudoStyle() is taking a "return 0" path.   While this code should protect itself against this case via something like 

           if (s && s != style())

it's not clear that this is sufficient; would like to know why it's not finding any style.

[Thomas Sepez]

adding the check :
  
    if (s && s != style())

merely moves the segv to RenderBlock::baselinePosition() where another deref occurs:

   const FontMetrics& fontMetrics = style(firstLine)->fontMetrics();

[Patrick R. Gansterer]

Created attachment 80565 [details]
LayoutTest

[Alexey Proskuryakov]

Crashes Safari 5.0.3 as well as nightlies in RenderBlock::findNextLineBreak().

[Alice Boxhall]

I've been looking into this with Ojan.

From what we can gather, when the body element gets attached, the :empty:first-line selector matches it (since nothing has been attached to it yet).

Then, when the button focus happens, the styles are not recalculated, so the hasPseudoStyle(FIRST_LINE) method returns true, but no style is matched, so a null pointer is returned to the findNextLineBreak (http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp?rev=80288#L1655) method.

(The original stack trace breaks in a different point, but I think it's likely to be the same cause.)

The new stack, from attach():

#0	0x10173ec94 in WTF::RefPtr<WebCore::StyleRareNonInheritedData>::get at RefPtr.h:60
#1	0x10169627d in WebCore::DataRef<WebCore::StyleRareNonInheritedData>::get at DataRef.h:33
#2	0x101696295 in WebCore::DataRef<WebCore::StyleRareNonInheritedData>::operator-> at DataRef.h:36
#3	0x10173ed95 in WebCore::RenderStyle::textCombine at RenderStyle.h:735
#4	0x101f8b071 in WebCore::RenderStyle::hasTextCombine at RenderStyle.h:736
#5	0x101f8309c in WebCore::RenderBlock::findNextLineBreak at RenderBlockLineLayout.cpp:1672
#6	0x101f882bd in WebCore::RenderBlock::layoutInlineChildren at RenderBlockLineLayout.cpp:692
#7	0x101f69527 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1224
#8	0x101f63ad8 in WebCore::RenderBlock::layout at RenderBlock.cpp:1122
#9	0x101f62bfc in WebCore::RenderBlock::layoutBlockChild at RenderBlock.cpp:1961
#10	0x101f68ecb in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1899
#11	0x101f69540 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1226
#12	0x101f63ad8 in WebCore::RenderBlock::layout at RenderBlock.cpp:1122
#13	0x101f62bfc in WebCore::RenderBlock::layoutBlockChild at RenderBlock.cpp:1961
#14	0x101f68ecb in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1899
#15	0x101f69540 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1226
#16	0x101f63ad8 in WebCore::RenderBlock::layout at RenderBlock.cpp:1122
#17	0x1020bd3a5 in WebCore::RenderView::layout at RenderView.cpp:130
#18	0x10196a1c6 in WebCore::FrameView::layout at FrameView.cpp:906
#19	0x1017a9a15 in WebCore::Document::updateLayout at Document.cpp:1588
#20	0x1018d0d1a in WebCore::Element::focus at Element.cpp:1498
#21	0x101a3ca93 in WebCore::HTMLFormControlElement::attach at HTMLFormControlElement.cpp:144

[Alice Boxhall]

Created attachment 85128 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 85128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85128&action=review

release the attack drones!

> Source/WebCore/ChangeLog:17
> +        (WebCore::checkForEmptyStyleChange): Pull out empty style checking
> +                                             logic from checkForSiblingStyleChanges().

darin yelled at me when I did funky alignments like that.

[Ojan Vafai]

Comment on attachment 85128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85128&action=review

>> Source/WebCore/ChangeLog:17
>> +                                             logic from checkForSiblingStyleChanges().
> 
> darin yelled at me when I did funky alignments like that.

What did he say to do instead?

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 85128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85128&action=review
> 
> >> Source/WebCore/ChangeLog:17
> >> +                                             logic from checkForSiblingStyleChanges().
> > 
> > darin yelled at me when I did funky alignments like that.
> 
> What did he say to do instead?

like this: http://trac.webkit.org/changeset/80604/trunk/Source/WebCore/ChangeLog
or also: http://trac.webkit.org/changeset/79607

[Alice Boxhall]

Created attachment 85235 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 85235 [details]
Patch

Make it so.

[WebKit Commit Bot]

Comment on attachment 85235 [details]
Patch

Clearing flags on attachment: 85235

Committed r80803: <http://trac.webkit.org/changeset/80803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.