Summary: | Uninitialized Value object when parsing '%' crashes an optimized ARM build | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick <phanna> | ||||||
Component: | CSS | Assignee: | 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
Patrick
2008-04-16 12:28:28 PDT
Created attachment 20599 [details]
Patch for CSSGrammar.y
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?
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? > 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? > 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). 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
Can someone with commit access land this patch for me? 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. Created attachment 21063 [details]
Changed test result
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.
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. 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. |