Bug 45081

Summary: Crash rendering <meter/> with percent padding
Product: WebKit Reporter: James Kozianski <koz>
Component: Layout and RenderingAssignee: James Kozianski <koz>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jamesr, morrita, noel.gordon, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Reproduction
none
Stack trace
none
Proposed patch
tkent: review-
Proposed patch
abarth: review-
Proposed patch none

Description James Kozianski 2010-09-01 19:09:44 PDT
Created attachment 66312 [details]
Reproduction

WebKit crashes when trying to render a <meter> element that has percentage padding, eg: <meter style="padding: 1%;" />

Test case attached.
Comment 1 James Kozianski 2010-09-01 20:46:18 PDT
Created attachment 66318 [details]
Stack trace
Comment 2 James Kozianski 2010-09-01 21:22:24 PDT
The crash is caused by a null dereference of containingBlock() in RenderBoxModelObject::paddingTop().

RenderMeter queries its size when determining whether it needs to be layed out, but initially it has no containing block, hence the null dereference.

I'll write a patch to make RenderMeter always request layout so this query never occurs.
Comment 3 James Kozianski 2010-09-02 01:22:10 PDT
Created attachment 66338 [details]
Proposed patch
Comment 4 WebKit Review Bot 2010-09-02 01:24:17 PDT
Attachment 66338 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Hajime Morrita 2010-09-02 01:47:35 PDT
Comment on attachment 66338 [details]
Proposed patch

Hi, thank you for doing this! 
The change looks OK in general. Please fix what the bot claims.
You can use WebKitTools/Scripts/check-webkit-style
to correct coding convention errors before the bot bites us.

For the test, are we OK even for other than padding?
Having size-based properties like margin, width, height would be helpful.

For ChangeLog, please mention what caused the crash briefly.
Comment 6 James Kozianski 2010-09-02 01:51:35 PDT
Created attachment 66339 [details]
Proposed patch
Comment 7 Kent Tamura 2010-09-02 01:52:03 PDT
Comment on attachment 66338 [details]
Proposed patch

r- for the style error.
Comment 8 Kent Tamura 2010-09-05 23:32:45 PDT
The second patch seems not to answer Morita-san's requests.

> For the test, are we OK even for other than padding?
> Having size-based properties like margin, width, height would be helpful.
> 
> For ChangeLog, please mention what caused the crash briefly.
Comment 9 Adam Barth 2010-09-05 23:43:39 PDT
Comment on attachment 66339 [details]
Proposed patch

See comment above.
Comment 10 James Kozianski 2010-09-06 21:29:27 PDT
Created attachment 66684 [details]
Proposed patch
Comment 11 Hajime Morrita 2010-09-07 00:49:30 PDT
Looks fine for me.
Comment 12 Kent Tamura 2010-09-07 00:50:21 PDT
Comment on attachment 66684 [details]
Proposed patch

OK.
Comment 13 WebKit Commit Bot 2010-09-07 01:12:45 PDT
Comment on attachment 66684 [details]
Proposed patch

Clearing flags on attachment: 66684

Committed r66864: <http://trac.webkit.org/changeset/66864>
Comment 14 WebKit Commit Bot 2010-09-07 01:12:49 PDT
All reviewed patches have been landed.  Closing bug.