Bug 127029 – SubresourceLoader::didFinishLoading() should not assert when a decode error occurs

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Debug iOS WebKit in the Simulator loading apple.com produces:
ASSERT(!m_resource->errorOccurred()); in WebCore::SubresourceLoader::didFinishLoading

--

lldb> p m_resource->status()
(WebCore::CachedResource::Status) $23 = DecodeError

-- 

    bool CachedImage::canBeDrawn() const
    {
        if (!m_image || m_image->isNull())
            return false;

        if (!m_loader || m_loader->reachedTerminalState())
            return true;

        size_t estimatedDecodedImageSize = m_image->width() * m_image->height() * 4; // no overflow check
        return estimatedDecodedImageSize <= m_loader->frameLoader()->frame().settings().maximumDecodedImageSize();
    }

--

lldb> p m_resource->url().m_string.m_impl.m_ptr
(WTF::StringImpl *const) $21 = 0x000000011a2643c0 { length = 63, is8bit = 1, contents = 'http://images.apple.com/v/home/am/images/your_verse_hero_2x.jpg' }

lldb> p m_loader->frameLoader()->frame().settings().maximumDecodedImageSize();
20971520

lldb> p m_image->width() * m_image->height() * 4
27302400

--

Is the maximumDecodedImageSize smaller then it should be, or is this a valid path and the ASSERT should be handled appropriately?

[Joseph Pecoraro]

> Is the maximumDecodedImageSize smaller then it should be

No. This is 20 * 1024 * 1024, which is the expected size on iOS.


> or is this a valid path and the ASSERT should be handled appropriately?

I think this is the issue. I think a DecodeError should probably have been handled higher up in the stack to not reach this ASSERT. The reason this ASSERT is not seen on Mac is that there is no max decoded size, and such a DecodeError never gets triggered.

[Alexey Proskuryakov]

Would WebKit hit this assertion on Mac if there is a true decode error (e.g. garbage bytes instead of an image resource)? If not, what prevents that from happening?

That said, it seems strange that a platform limitation like this is implemented via simulating a load failure.

[Daniel Bates]

(In reply to comment #2)
> Would WebKit hit this assertion on Mac if there is a true decode error (e.g. garbage bytes instead of an image resource)? 

No.

> If not, what prevents that from happening?

Either decoding happens after the resource is loaded (e.g. a font) or, in the case of a CachedImage, we don't explicitly query the status of the image decoder and hence don't update the status of the CachedResource (i.e. CachedResource::setStatus()) when an actual decoder error occurs. CachedImage is implicitly (*) informed about a decode error by the return value of calling Image::setData() in CachedImage::addIncrementalDataBuffer(), <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedImage.cpp?rev=161309#L406> and CachedImage::finishLoading(), <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedImage.cpp?rev=161309#L447>. As aforementioned, we don't use this return value to update the status of the resource.

(*) Looking at the control flow for a bitmap image, Image::setData() => BitmapImage::dataChanged() => ImageSource::isSizeAvailable() => BitmapImage::isSizeAvailable() => ImageSource::isSizeAvailable(). And ImageSource::isSizeAvailable() (as defined in ImageSourceCG.cpp) will return false if the status of the decoder is EOF was encountered unexpectedly (kCGImageStatusUnexpectedEOF), invalid data (kCGImageStatusInvalidData), the image is an unknown type (kCGImageStatusUnknownType), the decoder is in the process of reading the header of the image (kCGImageStatusReadingHeader), and "the operation is not complete" (kCGImageStatusIncomplete). See <https://developer.apple.com/library/ios/documentation/graphicsimaging/reference/CGImageSource/Reference/reference.html> for more details on the aforementioned status codes.

> That said, it seems strange that a platform limitation like this is implemented via simulating a load failure.

This platform limitation doesn't trigger a load failure therefore the assertion failure. That is, when a decode error occurs we continue the load through to completion and hence SubresourceLoader::didFinishLoading() is called.

[Alexey Proskuryakov]

<rdar://problem/16093385>

[Daniel Bates]

Created attachment 225292 [details]
Work-in-progress patch

Work-in-progress patch. There are some FIXME comments that need to be addressed in this patch. I'm not happy with this patch.

[Daniel Bates]

Created attachment 225323 [details]
Patch and Layout test

[Daniel Bates]

After speaking with Alexey Proskuryakov in-person today (02/26) we may want to consider removing the setting Settings::maximumDecodedImageSize() and related logic since the logic for this feature is broken. Specifically, when a maximum decoded image size is specified and an image exceeds it during an incremental load we don't cancel the load, see bug #129394. Additionally, when we determine that an image exceeds the maximum decoded image size on load completion we allow it to be decoded (though we may use subsampling) instead of preventing it from being decoded. Although when an image exceeds the maximum decoded image size we remove it from the memory cache, Simon Fraser raised a concern that we may subsequently download it again (and fail with the same decoding error) because we remove it from the memory cache without telling WebKit to prevent subsequent loads for the image. A minor issue is that Settings::maximumDecodedImageSize() seems too stringent for image documents. That is, loading an image file directly.

[Daniel Bates]

Committed r164863: <http://trac.webkit.org/changeset/164863>