Bug 170530 – CachedImage should stop decoding images with unknown types

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Currently we have to wait until the image size is available to make this decision. With malformed images, the size, which will be (0, 0) in this case, will be available only when all the image data is received.

[Said Abou-Hallawa]

Created attachment 306348 [details]
Patch

[Said Abou-Hallawa]

Created attachment 306352 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31468565>

[Radar WebKit Bug Importer]

<rdar://problem/31468566>

[Simon Fraser (smfr)]

Comment on attachment 306352 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:451
> +    EncodedDataStatus status = m_decoder->encodedDataStatus();
> +    if (status < EncodedDataStatus::Complete)
> +        return status;
> +    
> +    m_encodedDataStatus = status;
>      didDecodeProperties(m_decoder->bytesDecodedToDetermineProperties());

Doesn't this change mean that you'll never call didDecodeProperties(m_decoder->bytesDecodedToDetermineProperties()); before you've downloaded all the data (status < EncodedDataStatus::Complete)? Seems like a behavior change.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:195
> +            return EncodedDataStatus::TypeAvailable;

This assumes implicitly ordering in ImageIO (that TypeAvailable is always true even when data is incomplete). I hope that's true.

[Build Bot]

Comment on attachment 306352 [details]
Patch

Attachment 306352 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3482117

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 306354 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306352 [details]
Patch

Attachment 306352 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3482128

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 306355 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306352 [details]
Patch

Attachment 306352 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3482270

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 306358 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 306739 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 306352 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:451
>>      didDecodeProperties(m_decoder->bytesDecodedToDetermineProperties());
> 
> Doesn't this change mean that you'll never call didDecodeProperties(m_decoder->bytesDecodedToDetermineProperties()); before you've downloaded all the data (status < EncodedDataStatus::Complete)? Seems like a behavior change.

Yes you are right. I fixed that by the following logic:

-- Don't cache the encodedDataStatus unless the status is complete.
-- Don't call didDecodeProperties() until the status >= EncodedDataStatus::SizeAvailable

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:195
>> +            return EncodedDataStatus::TypeAvailable;
> 
> This assumes implicitly ordering in ImageIO (that TypeAvailable is always true even when data is incomplete). I hope that's true.

Yes this is true. CG moves the status of the CGImageSource to kCGImageStatusIncomplete only when the image reader is created. And the image reader is created only when the type is available.

[Tim Horton]

Comment on attachment 306739 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:2503
> +		5550CB421E955E3C00111AA0 /* ImageDefs.h in Headers */ = {isa = PBXBuildFile; fileRef = 5550CB411E955E3C00111AA0 /* ImageDefs.h */; settings = {ATTRIBUTES = (Private, ); }; };

I don't love the name. There is no predecent for -Defs. Can we go with -Types instead, which we have a bunch of?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:-180
> -    // Ragnaros yells: TOO SOON! You have awakened me TOO SOON, Executus!

Awwww, this is a sad removal.

[Said Abou-Hallawa]

Created attachment 306756 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306756 [details]
Patch

Clearing flags on attachment: 306756

Committed r215211: <http://trac.webkit.org/changeset/215211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.