Bug 18538 – Uninitialized Value object when parsing '%' crashes an optimized ARM build

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Patrick]

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.

[Patrick]

Created attachment 20599 [details]
Patch for CSSGrammar.y

[Eric Seidel (no email)]

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?

[Patrick]

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?
> 

[Patrick]

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?
> 

[Maciej Stachowiak]

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).

[Patrick]

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 on attachment 20599 [details]
Patch for CSSGrammar.y

r=me

[Patrick]

Can someone with commit access land this patch for me?

[Rob Buis]

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.

[Mark Rowe (bdash)]

Created attachment 21063 [details]
Changed test result

[Mark Rowe (bdash)]

Dan, can you take a look at the changed result please?

Comment on attachment 20599 [details]
Patch for CSSGrammar.y

Sorry, changing to r-. WebKit should match other engines on that test.

[Robert Blaut]

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. 

[Patrick]

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.

WebKit no longer uses the CSS parser that had this issue.