Bug 170530 - CachedImage should stop decoding images with unknown types
Summary: CachedImage should stop decoding images with unknown types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-05 17:27 PDT by Said Abou-Hallawa
Modified: 2017-04-11 08:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (29.49 KB, patch)
2017-04-05 17:51 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (41.80 KB, patch)
2017-04-05 18:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (789.02 KB, application/zip)
2017-04-05 18:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.32 MB, application/zip)
2017-04-05 19:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (688.74 KB, application/zip)
2017-04-05 19:56 PDT, Build Bot
no flags Details
Patch (44.96 KB, patch)
2017-04-10 14:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (45.08 KB, patch)
2017-04-10 16:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-04-05 17:27:04 PDT
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.
Comment 1 Said Abou-Hallawa 2017-04-05 17:51:02 PDT
Created attachment 306348 [details]
Patch
Comment 2 Said Abou-Hallawa 2017-04-05 18:20:58 PDT
Created attachment 306352 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2017-04-05 18:22:08 PDT
<rdar://problem/31468565>
Comment 4 Radar WebKit Bug Importer 2017-04-05 18:22:10 PDT
<rdar://problem/31468566>
Comment 5 Simon Fraser (smfr) 2017-04-05 18:34:39 PDT
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.
Comment 6 Build Bot 2017-04-05 18:58:23 PDT
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.
Comment 7 Build Bot 2017-04-05 18:58:24 PDT
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
Comment 8 Build Bot 2017-04-05 19:08:29 PDT
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.
Comment 9 Build Bot 2017-04-05 19:08:31 PDT
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
Comment 10 Build Bot 2017-04-05 19:56:16 PDT
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.
Comment 11 Build Bot 2017-04-05 19:56:17 PDT
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
Comment 12 Said Abou-Hallawa 2017-04-10 14:00:29 PDT
Created attachment 306739 [details]
Patch
Comment 13 Said Abou-Hallawa 2017-04-10 14:05:37 PDT
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.
Comment 14 Tim Horton 2017-04-10 15:56:38 PDT
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.
Comment 15 Said Abou-Hallawa 2017-04-10 16:35:40 PDT
Created attachment 306756 [details]
Patch
Comment 16 WebKit Commit Bot 2017-04-10 17:24:20 PDT
Comment on attachment 306756 [details]
Patch

Clearing flags on attachment: 306756

Committed r215211: <http://trac.webkit.org/changeset/215211>
Comment 17 WebKit Commit Bot 2017-04-10 17:24:22 PDT
All reviewed patches have been landed.  Closing bug.