67139 2011-08-29 10:37:13 -0700 Early return in CSSPrimitiveValue::getDoubleValueInternal() omits additional invalid enums 2011-09-14 11:57:14 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 ddkilzer apavlov apavlov macpherson simon.fraser oldest_to_newest 458273 0 ddkilzer 2011-08-29 10:37:13 -0700 * SUMMARY The early return code in CSSPrimitiveValue::getDoubleValueInternal() in CSSPrimitiveValue.cpp ignores the newly added CSS_COUNTER_NAME enum because it checks for currently-known illegal values instead of currently-known legal values. Current code: bool CSSPrimitiveValue::getDoubleValueInternal(UnitTypes requestedUnitType, double* result) const { if (m_type < CSS_NUMBER || (m_type > CSS_DIMENSION && m_type < CSS_TURN) || requestedUnitType < CSS_NUMBER || (requestedUnitType > CSS_DIMENSION && requestedUnitType < CSS_TURN)) return false; Instead it should do something like this: bool CSSPrimitiveValue::getDoubleValueInternal(UnitTypes requestedUnitType, double* result) const { if (!isValidCSSUnitTypeForDoubleConversion(m_type) || !isValidCSSUnitTypeForDoubleConversion(requestedUnitType)) return false; And then have a static inline method that handles all of them with a switch statement (so that you get a compiler warning if a new CSSPrimitiveValue::UnitTypes enum is added that isn't in the list: static inline bool isValidCSSUnitTypeForDoubleConversion(UnitTypes unitType) { switch (unitType) { case CSS_UNKNOWN: return false; case CSS_NUMBER: ... case CSS_DIMENSION: return true; case CSS_STRING: ... case CSS_PARSER_IDENTIFIER: return false; case CSS_TURN: case CSS_REMS: return true; case CSS_COUNTER_NAME: case CSS_FROM_FLOW: case CSS_SHAPE: return false; } ASSERT_NOT_REACHED(); return false; } The original code was added in ToT WebKit r72189. <http://trac.webkit.org/changeset/72189> 458275 1 ddkilzer 2011-08-29 10:38:00 -0700 (In reply to comment #0) > * SUMMARY > The early return code in CSSPrimitiveValue::getDoubleValueInternal() in CSSPrimitiveValue.cpp ignores the newly added CSS_COUNTER_NAME enum because it checks for currently-known illegal values instead of currently-known legal values. As well as CSS_FROM_FLOW and CSS_SHAPE now. 458974 2 105643 apavlov 2011-08-30 09:51:02 -0700 Created attachment 105643 [PATCH] Suggested fix Thanks for the tip David. 458976 3 105643 darin 2011-08-30 09:53:43 -0700 Comment on attachment 105643 [PATCH] Suggested fix View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review r=me even though I think we can make some small improvements > Source/WebCore/css/CSSPrimitiveValue.cpp:53 > + switch (unitType) { Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that. > Source/WebCore/css/CSSPrimitiveValue.cpp:86 > + case CSSPrimitiveValue:: CSS_UNICODE_RANGE: > + > + case CSSPrimitiveValue:: CSS_PARSER_OPERATOR: This blank line looks like a mistake. 459081 4 apavlov 2011-08-30 12:21:40 -0700 (In reply to comment #3) > (From update of attachment 105643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review > > r=me even though I think we can make some small improvements > > > Source/WebCore/css/CSSPrimitiveValue.cpp:53 > > + switch (unitType) { > > Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that. > > > Source/WebCore/css/CSSPrimitiveValue.cpp:86 > > + case CSSPrimitiveValue:: CSS_UNICODE_RANGE: > > + > > + case CSSPrimitiveValue:: CSS_PARSER_OPERATOR: > > This blank line looks like a mistake. Thanks Darin, I will land the patch tomorrow with these nits fixed. 459537 5 apavlov 2011-08-31 03:19:43 -0700 Committed r94169: <http://trac.webkit.org/changeset/94169> 467198 6 ddkilzer 2011-09-14 11:57:14 -0700 FWIW, I think a test could have been written for this using the fact that certain CSS properties would return a value instead of throwing an exception. 105643 2011-08-30 09:51:02 -0700 2011-08-30 09:53:43 -0700 [PATCH] Suggested fix fixprimitive.patch text/plain 4056 apavlov Y29tbWl0IGI4NTQ1NDZjNWM3ZWMyNzIxMDk4N2NjNWY3OWFmMDk0Y2I2OWI2MDYKQXV0aG9yOiBB bGV4YW5kZXIgUGF2bG92IDxhcGF2bG92QGNocm9taXVtLm9yZz4KRGF0ZTogICBUdWUgQXVnIDMw IDIwOjQ2OjEyIDIwMTEgKzA0MDAKCiAgICBGaXgKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWI1NjJhNC4uOGM3 MjViZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDExLTA4LTMwICBBbGV4YW5kZXIg UGF2bG92ICA8YXBhdmxvdkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgRWFybHkgcmV0dXJuIGlu IENTU1ByaW1pdGl2ZVZhbHVlOjpnZXREb3VibGVWYWx1ZUludGVybmFsKCkgb21pdHMgYWRkaXRp b25hbCBpbnZhbGlkIGVudW1zCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD02NzEzOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgICogY3NzL0NTU1ByaW1pdGl2ZVZhbHVlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OmlzVmFsaWRDU1NVbml0VHlwZUZvckRvdWJsZUNvbnZlcnNpb24pOiBDaGVjayBpZiBhIGdpdmVu IHVuaXQgdHlwZSBjYW4gYmUgY29udmVydGVkIHRvIGEgZG91YmxlLgorICAgICAgICAoV2ViQ29y ZTo6Q1NTUHJpbWl0aXZlVmFsdWU6OmdldERvdWJsZVZhbHVlSW50ZXJuYWwpOiBNYWtlIHVzZSBv ZiB0aGUgbmV3IG1ldGhvZCB0aGF0IGV4cGxpY2l0bHkgbGlzdHMgYWxsIHVuaXQgdHlwZXMuCisK IDIwMTEtMDgtMzAgIEthdXN0dWJoIEF0cmF3YWxrYXIgIDxrYXVzdHViaEBtb3Rvcm9sYS5jb20+ CiAKICAgICAgICAgVGhlIHVudXNlZCBTY3JvbGxWaWV3KiBhcmd1bWVudCBjYW4gYW5kIHNob3Vs ZCBiZSByZW1vdmVkIGZyb20KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NQcmlt aXRpdmVWYWx1ZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTUHJpbWl0aXZlVmFsdWUuY3Bw CmluZGV4IDk4MTZkYTUuLjJjYjU3YzMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9D U1NQcmltaXRpdmVWYWx1ZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvY3NzL0NTU1ByaW1pdGl2 ZVZhbHVlLmNwcApAQCAtNDgsNiArNDgsNTkgQEAgdXNpbmcgbmFtZXNwYWNlIFdURjsKIAogbmFt ZXNwYWNlIFdlYkNvcmUgewogCitzdGF0aWMgaW5saW5lIGJvb2wgaXNWYWxpZENTU1VuaXRUeXBl Rm9yRG91YmxlQ29udmVyc2lvbihDU1NQcmltaXRpdmVWYWx1ZTo6VW5pdFR5cGVzIHVuaXRUeXBl KQoreworICAgIHN3aXRjaCAodW5pdFR5cGUpIHsKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZhbHVl OjogQ1NTX1VOS05PV046CisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICBjYXNlIENTU1ByaW1p dGl2ZVZhbHVlOjogQ1NTX05VTUJFUjoKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZhbHVlOjogQ1NT X1BFUkNFTlRBR0U6CisgICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19FTVM6CisgICAg Y2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19FWFM6CisgICAgY2FzZSBDU1NQcmltaXRpdmVW YWx1ZTo6IENTU19QWDoKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZhbHVlOjogQ1NTX0NNOgorICAg IGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfTU06CisgICAgY2FzZSBDU1NQcmltaXRpdmVW YWx1ZTo6IENTU19JTjoKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZhbHVlOjogQ1NTX1BUOgorICAg IGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfUEM6CisgICAgY2FzZSBDU1NQcmltaXRpdmVW YWx1ZTo6IENTU19ERUc6CisgICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19SQUQ6Cisg ICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19HUkFEOgorICAgIGNhc2UgQ1NTUHJpbWl0 aXZlVmFsdWU6OiBDU1NfTVM6CisgICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19TOgor ICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfSFo6CisgICAgY2FzZSBDU1NQcmltaXRp dmVWYWx1ZTo6IENTU19LSFo6CisgICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19ESU1F TlNJT046CisgICAgICAgIHJldHVybiB0cnVlOworICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6 OiBDU1NfU1RSSU5HOgorICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfVVJJOgorICAg IGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfSURFTlQ6CisgICAgY2FzZSBDU1NQcmltaXRp dmVWYWx1ZTo6IENTU19BVFRSOgorICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfQ09V TlRFUjoKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZhbHVlOjogQ1NTX1JFQ1Q6CisgICAgY2FzZSBD U1NQcmltaXRpdmVWYWx1ZTo6IENTU19SR0JDT0xPUjoKKyAgICBjYXNlIENTU1ByaW1pdGl2ZVZh bHVlOjogQ1NTX1BBSVI6CisgICAgY2FzZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19EQVNIQk9B UkRfUkVHSU9OOgorICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfVU5JQ09ERV9SQU5H RToKKworICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfUEFSU0VSX09QRVJBVE9SOgor ICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfUEFSU0VSX0lOVEVHRVI6CisgICAgY2Fz ZSBDU1NQcmltaXRpdmVWYWx1ZTo6IENTU19QQVJTRVJfSEVYQ09MT1I6CisgICAgY2FzZSBDU1NQ cmltaXRpdmVWYWx1ZTo6IENTU19QQVJTRVJfSURFTlRJRklFUjoKKyAgICAgICAgcmV0dXJuIGZh bHNlOworICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfVFVSTjoKKyAgICBjYXNlIENT U1ByaW1pdGl2ZVZhbHVlOjogQ1NTX1JFTVM6CisgICAgICAgIHJldHVybiB0cnVlOworICAgIGNh c2UgQ1NTUHJpbWl0aXZlVmFsdWU6OiBDU1NfQ09VTlRFUl9OQU1FOgorICAgIGNhc2UgQ1NTUHJp bWl0aXZlVmFsdWU6OiBDU1NfRlJPTV9GTE9XOgorICAgIGNhc2UgQ1NTUHJpbWl0aXZlVmFsdWU6 OiBDU1NfU0hBUEU6CisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CisKKyAgICBBU1NFUlRf Tk9UX1JFQUNIRUQoKTsKKyAgICByZXR1cm4gZmFsc2U7Cit9CisKIHN0YXRpYyBDU1NQcmltaXRp dmVWYWx1ZTo6VW5pdENhdGVnb3J5IHVuaXRDYXRlZ29yeShDU1NQcmltaXRpdmVWYWx1ZTo6VW5p dFR5cGVzIHR5cGUpCiB7CiAgICAgLy8gSGVyZSB3ZSB2aW9sYXRlIHRoZSBzcGVjIChodHRwOi8v d3d3LnczLm9yZy9UUi9ET00tTGV2ZWwtMi1TdHlsZS9jc3MuaHRtbCNDU1MtQ1NTUHJpbWl0aXZl VmFsdWUpIGFuZCBhbGxvdyBjb252ZXJzaW9ucwpAQCAtNDUyLDcgKzUwNSw3IEBAIENTU1ByaW1p dGl2ZVZhbHVlOjpVbml0VHlwZXMgQ1NTUHJpbWl0aXZlVmFsdWU6OmNhbm9uaWNhbFVuaXRUeXBl Rm9yQ2F0ZWdvcnkoVW5pCiAKIGJvb2wgQ1NTUHJpbWl0aXZlVmFsdWU6OmdldERvdWJsZVZhbHVl SW50ZXJuYWwoVW5pdFR5cGVzIHJlcXVlc3RlZFVuaXRUeXBlLCBkb3VibGUqIHJlc3VsdCkgY29u c3QKIHsKLSAgICBpZiAobV90eXBlIDwgQ1NTX05VTUJFUiB8fCAobV90eXBlID4gQ1NTX0RJTUVO U0lPTiAmJiBtX3R5cGUgPCBDU1NfVFVSTikgfHwgcmVxdWVzdGVkVW5pdFR5cGUgPCBDU1NfTlVN QkVSIHx8IChyZXF1ZXN0ZWRVbml0VHlwZSA+IENTU19ESU1FTlNJT04gJiYgcmVxdWVzdGVkVW5p dFR5cGUgPCBDU1NfVFVSTikpCisgICAgaWYgKCFpc1ZhbGlkQ1NTVW5pdFR5cGVGb3JEb3VibGVD b252ZXJzaW9uKHN0YXRpY19jYXN0PFVuaXRUeXBlcz4obV90eXBlKSkgfHwgIWlzVmFsaWRDU1NV bml0VHlwZUZvckRvdWJsZUNvbnZlcnNpb24ocmVxdWVzdGVkVW5pdFR5cGUpKQogICAgICAgICBy ZXR1cm4gZmFsc2U7CiAgICAgaWYgKHJlcXVlc3RlZFVuaXRUeXBlID09IG1fdHlwZSB8fCByZXF1 ZXN0ZWRVbml0VHlwZSA9PSBDU1NfRElNRU5TSU9OKSB7CiAgICAgICAgICpyZXN1bHQgPSBtX3Zh bHVlLm51bTsK