Bug 61989

Summary: REGRESSION (r82580): Reproducible crash in CSSPrimitiveValue::computeLengthDouble
Product: WebKit Reporter: David Barr <davidbarr>
Component: CSSAssignee: David Barr <davidbarr>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, davidbarr, eric, evan, koivisto, leviw, macpherson, mikelawther, mitz, parag, simon.fraser, webkit.review.bot
Priority: P1 Keywords: Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://crbug.com/83633
Attachments:
Description Flags
Full stack trace from chromium crash
none
Proposed Patch
none
Updated Patch none

David Barr
Reported 2011-06-02 20:18:33 PDT
Minimal repro: -- <html hidden><title style="top: 1rem;">a</title></html> -- Null-deref evaluating rootStyle->fontDescription() in WebCore::CSSPrimitiveValue::computeLengthDouble(). When parsing any CSS length with 'rem' as the units within the element style of the title, if the html element is hidden, rootElementStyle is undefined and a null-deref occurs when attempting to derive the font size.
Attachments
Full stack trace from chromium crash (8.04 KB, text/plain)
2011-06-02 20:19 PDT, David Barr 		no flags
Details
Proposed Patch (3.76 KB, patch)
2012-01-30 06:04 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Updated Patch (3.71 KB, patch)
2012-01-30 07:11 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
David Barr
Comment 1 2011-06-02 20:19:36 PDT
Created attachment 95848 [details] Full stack trace from chromium crash
Mike Lawther
Comment 2 2011-06-02 20:29:55 PDT
Crashes WebKit Nightly (r87828) and Chrome Canary (13.0.782.3 canary) on Mac
Alexey Proskuryakov
Comment 3 2011-06-02 22:26:57 PDT
Confirmed using r87633. Does not crash Safari 5.0.5. Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000040 Crashed Thread: 0 Dispatch queue: com.apple.main-thread Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010381e3e4 WTF::RefPtr<WebCore::StyleInheritedData>::get() const + 12 (RefPtr.h:60) 1 com.apple.WebCore 0x00000001038ad2c9 WebCore::DataRef<WebCore::StyleInheritedData>::get() const + 21 (DataRef.h:33) 2 com.apple.WebCore 0x00000001038ad2e1 WebCore::DataRef<WebCore::StyleInheritedData>::operator->() const + 21 (DataRef.h:36) 3 com.apple.WebCore 0x00000001038ad2fd WebCore::RenderStyle::fontDescription() const + 25 (RenderStyle.h:479) 4 com.apple.WebCore 0x000000010388bcb4 WebCore::CSSPrimitiveValue::computeLengthDouble(WebCore::RenderStyle*, WebCore::RenderStyle*, double, bool) + 330 (CSSPrimitiveValue.cpp:319) 5 com.apple.WebCore 0x000000010388bee4 WebCore::CSSPrimitiveValue::computeLengthIntForLength(WebCore::RenderStyle*, WebCore::RenderStyle*, double) + 52 (CSSPrimitiveValue.cpp:271) 6 com.apple.WebCore 0x00000001038b6486 WebCore::ApplyPropertyLength<(WebCore::LengthAuto)1, (WebCore::LengthIntrinsic)0, (WebCore::LengthMinIntrinsic)0, (WebCore::LengthNone)0, (WebCore::LengthUndefined)0>::applyValue(WebCore::CSSStyleSelector*, WebCore::CSSValue*) const + 504 (CSSStyleApplyProperty.cpp:232) 7 com.apple.WebCore 0x00000001038e3a79 WebCore::CSSStyleApplyProperty::applyValue(WebCore::CSSPropertyID, WebCore::CSSStyleSelector*, WebCore::CSSValue*) const + 131 (CSSStyleApplyProperty.h:68) ...
Mike Lawther
Comment 4 2011-06-03 00:00:56 PDT
The bug seems to apply to any property that references m_rootElementStyle, eg <html hidden><title style="line-height: 1rem;">a</title></html> also causes a crash. In eliminating possible culprits, we've confirmed the crash occurs on MacOS as far back as Webkit r85429 (using r85429.dmg">http://builds.nightly.webkit.org/files/trunk/mac/WebKit-SVN-r85429.dmg).
Luke Macpherson
Comment 5 2011-06-05 18:36:26 PDT
I did a binary search of chromium continuous build revisions using the supplied test case. The first breakage was between chromium revisions 79962 and 80134. These correspond to webkit revisions 82507 and 82657 respectively.
Alexey Proskuryakov
Comment 6 2011-06-05 21:27:13 PDT
Works: r82578 Fails: r82580.
Evan Martin
Comment 7 2011-06-06 08:11:20 PDT
Just to give some background: - when we see a <title> tag, we attempt to compute the text of the tag - when we compute the text, we use the computed style to see if it's LTR or RTL - this 'hidden' attr is probably making some style-related computation fail
Parag Radke
Comment 8 2012-01-30 06:04:18 PST
Created attachment 124539 [details] Proposed Patch Added a null check for RenderStyle of root element while calculating length for css3 rems unit
Simon Fraser (smfr)
Comment 9 2012-01-30 06:12:03 PST
Comment on attachment 124539 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124539&action=review > Source/WebCore/ChangeLog:9 > + of the root element.In this case as HTML element has a property 'hidden' and hence renderer is NULL Space after . please. > Source/WebCore/ChangeLog:12 > + Test: fast/css/bug61989-fontsize-unit-rems-crash.html You don't really need the bug number in the filename. > LayoutTests/fast/css/bug61989-fontsize-unit-rems-crash.html:1 > +<html hidden><title style="line-height: 1rem;">Test case for 61989</title> The hidden attribute seems contentious <http://peter.sh/2010/06/thoughts-on-the-html5-hidden-attribute/> Does the bug still reproduce with display:none? That might be a better way to test it.
Parag Radke
Comment 10 2012-01-30 07:11:46 PST
Created attachment 124544 [details] Updated Patch Updated patch with all review comments implemented
WebKit Review Bot
Comment 11 2012-01-30 09:32:08 PST
Comment on attachment 124544 [details] Updated Patch Clearing flags on attachment: 124544 Committed r106251: <http://trac.webkit.org/changeset/106251>
WebKit Review Bot
Comment 12 2012-01-30 09:32:13 PST
All reviewed patches have been landed. Closing bug.
Levi Weintraub
Comment 13 2012-01-30 11:02:13 PST
(In reply to comment #12) > All reviewed patches have been landed. Closing bug. Is there any reason we're not using dumpAsText on the fast/css/fontsize-unit-rems-crash.html layout test?
Parag Radke
Comment 14 2012-01-31 03:00:45 PST
> Is there any reason we're not using dumpAsText on the fast/css/fontsize-unit-rems-crash.html layout test? yes, Actually when i was using dumpAsText on fast/css/fontsize-unit-rems-crash.html it was dumping all the content of the html page including the script as the test case has html property hidden.
Levi Weintraub
Comment 15 2012-01-31 10:02:19 PST
(In reply to comment #14) > > Is there any reason we're not using dumpAsText on the fast/css/fontsize-unit-rems-crash.html layout test? > > yes, Actually when i was using dumpAsText on fast/css/fontsize-unit-rems-crash.html it was dumping all the content of the html page including the script as the test case has html property hidden. Is this an issue for test coverage?
Parag Radke
Comment 16 2012-02-01 03:17:55 PST
> Is this an issue for test coverage? I am not sure if this is an issue with dumprendertree but when i was using it with this test case i was getting the expected output as Test case for 61989 This is test for Bug 61989 No crash means test PASS.if (window.layoutTestController) layoutTestController.dumpAsText(); which includes the title text and script content as well.
Note You need to log in before you can comment on or make changes to this bug.