Bug 18538

Summary: Uninitialized Value object when parsing '%' crashes an optimized ARM build
Product: WebKit Reporter: Patrick <phanna>
Component: CSSAssignee: mitz
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: gyuyoung, webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Patch for CSSGrammar.y
mitz: review-
Changed test result none

Description Patrick 2008-04-16 12:28:28 PDT
Changelist 26482 introduced the ability to skip attribute values that are single '%' characters. This created an uninitialized Value object added to the valueList. This object caused a crash on an arm release build. I have a patch that I have tested against the original bug.
Comment 1 Patrick 2008-04-16 12:29:27 PDT
Created attachment 20599 [details]
Patch for CSSGrammar.y
Comment 2 Eric Seidel (no email) 2008-04-16 21:43:45 PDT
Comment on attachment 20599 [details]
Patch for CSSGrammar.y

Is it possible to make a test case for this?

Maybe:
width: %;

and then asking via getComputedStyle(element, null).width, or similar?
Comment 3 Patrick 2008-04-17 05:11:38 PDT
Sure. I'm not very familiar with writing a layout test though. Where should I put it?

(In reply to comment #2)
> (From update of attachment 20599 [details] [edit])
> Is it possible to make a test case for this?
> 
> Maybe:
> width: %;
> 
> and then asking via getComputedStyle(element, null).width, or similar?
> 

Comment 4 Patrick 2008-04-17 05:58:19 PDT
Looking through the codebase I found LayoutTests/fast/css/invalid-percentage-property.html which uses width: %;

Should I augment this test or is it sufficient?

(In reply to comment #2)
> (From update of attachment 20599 [details] [edit])
> Is it possible to make a test case for this?
> 
> Maybe:
> width: %;
> 
> and then asking via getComputedStyle(element, null).width, or similar?
> 

Comment 5 Maciej Stachowiak 2008-04-22 22:39:21 PDT
Does the test case you mention crash without your fix, or otherwise change in result with your fix? Ideally we want a test case that crashes without (but maybe the bug is latent).
Comment 6 Patrick 2008-04-23 10:39:14 PDT
Unfortunately this only crashed for me on an optimized arm build. It crashed because it tried to create a String from an uninitialized ParseString with a huge length. Safari does not crash in debug or release mode. The only way I found out why our port crashed is by using Valgrind on a linux debug build and then tracing through the code.

The test case doesn't render any different with or without my fix.
Comment 7 mitz 2008-04-29 10:21:05 PDT
Comment on attachment 20599 [details]
Patch for CSSGrammar.y

r=me
Comment 8 Patrick 2008-04-29 10:45:58 PDT
Can someone with commit access land this patch for me?
Comment 9 Rob Buis 2008-05-06 11:08:51 PDT
I get a changed end result for LayoutTests/fast/css/invalid-percentage-property.html. It does not look like an improvement, for starters it does
not match FF3 and Opera. Maybe Mitz can have a look.
Cheers,

Rob.
Comment 10 Mark Rowe (bdash) 2008-05-10 23:44:03 PDT
Created attachment 21063 [details]
Changed test result
Comment 11 Mark Rowe (bdash) 2008-05-10 23:45:05 PDT
Dan, can you take a look at the changed result please?
Comment 12 mitz 2008-05-12 11:59:33 PDT
Comment on attachment 20599 [details]
Patch for CSSGrammar.y

Sorry, changing to r-. WebKit should match other engines on that test.
Comment 13 Robert Blaut 2008-07-28 03:48:03 PDT
The proposed patch was completely incorrect. It forces WebKit to treat "width: %" like "width 0%" which it violates CSS 2.1 specification.. Current WebKit behavior is correct. I think we should close this bug report. 
Comment 14 Patrick 2008-07-28 05:02:26 PDT
Maybe treating '%' as '0%' is incorrect, but leaving an uninitialized variable can cause WebKit to crash. Instead of closing the bug, maybe there is another solution that does not violate the CSS 2.1 spec.
Comment 15 mitz 2017-03-25 19:09:08 PDT
WebKit no longer uses the CSS parser that had this issue.