Bug 84211 – Add default constructor for CSSImageSetValue::ImageWithScale to prevent uninitialized use.

• Status: NEW
• Component: CSS

[Luke Macpherson]

Add default constructor for CSSImageSetValue::ImageWithScale.

[Luke Macpherson]

Created attachment 137634 [details]
Patch

[Sam Weinig]

Comment on attachment 137634 [details]
Patch

I don't think this adds any real benefit.  THe initialization of imageURL is also completely unnecessary, as String has a default initialization already.  If there is a case where we are using this uninitialized, we should probably fix it at the call site.

[Luke Macpherson]

Created attachment 137712 [details]
Patch

[Luke Macpherson]

Thanks Sam, you were dead on - this patch just initializes in the appropriate place as you suggested.

[Andreas Kling]

Comment on attachment 137712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137712&action=review

> Source/WebCore/ChangeLog:13
> +        * css/CSSImageSetValue.h:
> +        (WebCore::CSSImageSetValue::ImageWithScale::ImageWithScale):

Outdated.

> Source/WebCore/css/CSSImageSetValue.cpp:80
> -    ImageWithScale image;
> +    ImageWithScale image = ImageWithScale();

This change is a no-op. image.scaleFactor will still contain uninitialized data.

[Luke Macpherson]

Comment on attachment 137712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137712&action=review

>> Source/WebCore/css/CSSImageSetValue.cpp:80
>> +    ImageWithScale image = ImageWithScale();
> 
> This change is a no-op. image.scaleFactor will still contain uninitialized data.

The documentation I looked at said that the default constructor wasn't called implicitly for structs, but I just ran some test cases in gcc and it did in fact call the constructor, so I guess that documentation was wrong. Don't believe everything you read on the internets :)

[Luke Macpherson]

Created attachment 137915 [details]
Patch

[Rafael Brandao]

Comment on attachment 137915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137915&action=review

> Source/WebCore/css/CSSImageSetValue.cpp:81
>      size_t numberOfImages = m_imagesInSet.size();

Can you explain when this size will be zero? According to http://lists.w3.org/Archives/Public/www-style/2012Feb/1103.html, it says it will receive one or more imagespec.

[Luke Macpherson]

Comment on attachment 137915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137915&action=review

>> Source/WebCore/css/CSSImageSetValue.cpp:81
>>      size_t numberOfImages = m_imagesInSet.size();
> 
> Can you explain when this size will be zero? According to http://lists.w3.org/Archives/Public/www-style/2012Feb/1103.html, it says it will receive one or more imagespec.

That may be expected, but I don't think that this code should assume that numberOfImages is > 0. At a bare minimum it should be asserted in debug mode, but for robustness it is even better to provide sensible behavior in that case.

[Eric Seidel (no email)]

Comment on attachment 137915 [details]
Patch

Seems reasonable.  There is no way currently to create an CSSImageSetValue with 0 images?

[Luke Macpherson]

(In reply to comment #10)
> (From update of attachment 137915 [details])
> Seems reasonable.  There is no way currently to create an CSSImageSetValue with 0 images?

It seems entirely possible if you consider the class in isolation - after construction CSSImageSetValue has 0 image, however apparently it is never called in that way - ie. images are always added before calling bestImageScaleFactor().

[Beth Dakin]

So just to be clear, does this fix correct an actual bug or just make the code more robust? Also, is there any way you can add a zero-paramter case to fast/css/image-set-parsing.html or fast/css/image-set-parsing-invalid.html to cover this?

[Mark Rowe (bdash)]

This is another instance of the "No new tests / code cleanup only." comment in the ChangeLog being a lie.

[Andreas Kling]

Comment on attachment 137915 [details]
Patch

If anything, we should add a default constructor for ImageWithScale.
This spot fix will not protect against someone doing "ImageWithScale foo;" somewhere else in the future and getting an uninitialized m_scaleFactor.