QuickLook resources are cache-replaced with their original binary data causing ASSERT(m_data->size() == newBuffer->size()) in CachedResource.cpp

[Pratik Solanki]

When loading QuickLook resources, the SharedBuffer in the CachedResource is actually a converted representation of the real quicklook resource. Later, when we get the notification from Networking process to replace the data with the file backed buffer, the assets in tryReplaceEncodedData() get triggered. We should disable the replacement of data for quicklook resources. It's possible that we may show corrupted data if we later tried to display the quicklook binary data. I have not been able to trigger this though.

<rdar://problem/17891321>

[Pratik Solanki]

Created attachment 235942 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 235942 [details]
Patch

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

r=me, but please removed the unused method disableEncodedDataReplacement() unless there is a missing source file for this patch!

> Source/WebCore/loader/ResourceLoader.cpp:67
> +#if USE(QUICK_LOOK)
> +    , m_quickLookResource(false)
> +#endif

It would make the code a bit more readable if you removed this code from USE(QUICK_LOOK) and only set m_quickLookResource to true in ResourceLoader::didCreateQuickLookHandle().

> Source/WebCore/loader/ResourceLoader.h:227
> +#if USE(QUICK_LOOK)
> +public:
> +    bool isQuickLookResource() { return m_quickLookResource; }
> +private:
> +    bool m_quickLookResource;
> +#endif

Ditto.

> Source/WebCore/loader/cache/CachedRawResource.cpp:45
> +#if USE(QUICK_LOOK)
> +    , m_allowEncodedDataReplacement(true)
> +#endif

It would make the code a bit more readable if you removed this code from USE(QUICK_LOOK) and only set m_allowEncodedDataReplacement to true in CachedRawResource::finishLoading().

> Source/WebCore/loader/cache/CachedRawResource.h:51
> +#if USE(QUICK_LOOK)
> +    void disableEncodedDataReplacement() { m_allowEncodedDataReplacement = false; }
> +#endif

This new method doesn't appear to be used anywhere?

> Source/WebCore/loader/cache/CachedRawResource.h:71
> +#if USE(QUICK_LOOK)
> +    virtual bool mayTryReplaceEncodedData() const override { return m_allowEncodedDataReplacement; }
> +#else
>      virtual bool mayTryReplaceEncodedData() const override { return true; }
> +#endif

It would make the code a bit more readable if you removed this code from USE(QUICK_LOOK) and only set m_allowEncodedDataReplacement to true in CachedRawResource::finishLoading().

> Source/WebCore/loader/cache/CachedRawResource.h:86
> +#if USE(QUICK_LOOK)
> +    bool m_allowEncodedDataReplacement;
> +#endif

Ditto.

[Daniel Bates]

Comment on attachment 235942 [details]
Patch

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

> Source/WebCore/loader/ResourceLoader.cpp:66
> +    , m_quickLookResource(false)

We prefix the name of the getter function with "m_" as use that as the name of the underlying instance variable. So, this should be named m_isQuickLookResource. From what I recall this also makes the name of the instance variable consistent with the Cocoa naming conventions (which we tend to take inspiration from in WebKit) when naming boolean variables and getters/setters.

[Pratik Solanki]

Committed r171993: <http://trac.webkit.org/changeset/171993>