Bug 53316 - NULL pointer crash when using :empty and :first-line pseudoclass selectors together
Summary: NULL pointer crash when using :empty and :first-line pseudoclass selectors to...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Critical
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2011-01-28 11:44 PST by Thomas Sepez
Modified: 2011-03-10 18:17 PST (History)
10 users (show)

See Also:


Attachments
LayoutTest (292 bytes, text/html)
2011-01-29 10:18 PST, Patrick R. Gansterer
no flags Details
Patch (4.71 KB, patch)
2011-03-08 20:37 PST, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2011-03-09 14:21 PST, Alice Boxhall
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 2011-01-28 11:44:33 PST
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

...
Comment 1 Thomas Sepez 2011-01-28 12:41:00 PST
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.
Comment 2 Thomas Sepez 2011-01-28 14:10:12 PST
adding the check :
  
    if (s && s != style())

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

   const FontMetrics& fontMetrics = style(firstLine)->fontMetrics();
Comment 3 Patrick R. Gansterer 2011-01-29 10:18:21 PST
Created attachment 80565 [details]
LayoutTest
Comment 4 Alexey Proskuryakov 2011-01-29 10:33:52 PST
Crashes Safari 5.0.3 as well as nightlies in RenderBlock::findNextLineBreak().
Comment 5 Alice Boxhall 2011-03-03 21:24:09 PST
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
Comment 6 Alice Boxhall 2011-03-08 20:37:00 PST
Created attachment 85128 [details]
Patch
Comment 7 Dimitri Glazkov (Google) 2011-03-08 20:58:08 PST
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.
Comment 8 Ojan Vafai 2011-03-08 21:03:30 PST
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?
Comment 9 Dimitri Glazkov (Google) 2011-03-08 21:10:09 PST
(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
Comment 10 Alice Boxhall 2011-03-09 14:21:37 PST
Created attachment 85235 [details]
Patch
Comment 11 Dimitri Glazkov (Google) 2011-03-09 14:49:01 PST
Comment on attachment 85235 [details]
Patch

Make it so.
Comment 12 WebKit Commit Bot 2011-03-10 18:17:21 PST
Comment on attachment 85235 [details]
Patch

Clearing flags on attachment: 85235

Committed r80803: <http://trac.webkit.org/changeset/80803>
Comment 13 WebKit Commit Bot 2011-03-10 18:17:26 PST
All reviewed patches have been landed.  Closing bug.