Crash in 3rd party WebKit apps that disable cache at a wrong time

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

A lot of memory cache code depends on subtle ways of keeping a cached resource alive. In particular, CachedResourceLoader::loadResource() both temporarily adds the resource to cache, and disables pruning, because the former was found to be ineffective.

That's not enough however, because a client still do things that would make the resource deleted, such as disable the cache.

<rdar://problem/10615880>

[Alexey Proskuryakov]

Created attachment 141040 [details]
proposed fix

The patch prepared as:
1. Revert WebCore part of r111272, leaving its test around.
2. Remove code that forced temporarily adding new resource to cache for "protection" even if caching was generally disabled.
3. Changed loadResource() to use CachedResourceHandle.
4. Fixed build breakage by deploying CachedResourceHandle elsewhere.
5. Added a FIXME in for a pre-existing issue in requestUserCSSStyleSheet().
6. Added an API test (and confirmed that it does check for this problem).

[Brady Eidson]

Comment on attachment 141040 [details]
proposed fix

This does look fine to me.  I still would like Antti to give it a pass, also.

[WebKit Review Bot]

Comment on attachment 141040 [details]
proposed fix

Attachment 141040 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12651734

[Antti Koivisto]

Comment on attachment 141040 [details]
proposed fix

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

r=me

> Source/WebCore/ChangeLog:12
> +        The fix is to use CacheResourceHandle throughout MemoryCache, which will certainly
> +        keep the resource alive. Also removed earlier fixes.

Seems like a good idea. The only worry is that using CachedResourceHandles as stack objects will create some HashMap churn in CachedResource::registerHandle/unregisterHandle for the revalidation case. I think that should be ok in practice though.

Typo CacheResourceHandle ->CachedResourceHandle

[Alexey Proskuryakov]

-        if (CachedImage* cachedImage = loader->requestImage(request)) {
+        if (CachedImage* cachedImage = loader->requestImage(request).get()) {
             m_imageSet = StyleCachedImageSet::create(cachedImage, image.scaleFactor, this);
             m_accessedBestFitImage = true;
         }

Having slept on it, I realize that this is wrong. When CachedResourceHandle is the only thing keeping resource alive, the object will be destroyed within the if block.

-                m_cachedSheet = document()->cachedResourceLoader()->requestXSLStyleSheet(request);
+                m_cachedSheet = document()->cachedResourceLoader()->requestXSLStyleSheet(request).get();

This would be better served by teaching CachedResourceHandle how to downcast from CachedXXX to plain CachedResource. Then I wouldn't have to use suspicious looking get().

New patch coming after testing.

[Alexey Proskuryakov]

Created attachment 141211 [details]
updated patch for landing

This has some changes from previous version, but probably not enough to need new review.

[WebKit Review Bot]

Comment on attachment 141211 [details]
updated patch for landing

Rejecting attachment 141211 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nk #5 succeeded at 679 (offset 14 lines).
Hunk #6 FAILED at 871.
1 out of 6 hunks FAILED -- saving rejects to file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj.rej
patching file Tools/TestWebKitAPI/Tests/mac/MemoryCacheDisableWithinResourceLoadDelegate.html
patching file Tools/TestWebKitAPI/Tests/mac/MemoryCacheDisableWithinResourceLoadDelegate.mm

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12647971

[Alexey Proskuryakov]

Created attachment 141241 [details]
updated to ToT

[WebKit Review Bot]

Comment on attachment 141241 [details]
updated to ToT

Rejecting attachment 141241 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 error: conversion from 'WebCore::CachedResourceHandle<WebCore::CachedShader>' to non-scalar type 'WebCore::CachedResourceLoader' requested
Source/WebCore/css/WebKitCSSShaderValue.cpp:62: error: could not convert 'cachedShader' to 'bool'
Source/WebCore/css/WebKitCSSShaderValue.cpp:63: error: 'class WebCore::CachedResourceLoader' has no member named 'get'
make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/css/WebKitCSSShaderValue.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12655995

[Alexey Proskuryakov]

Created attachment 141280 [details]
with chromium typo fix

[WebKit Review Bot]

Comment on attachment 141280 [details]
with chromium typo fix

Clearing flags on attachment: 141280

Committed r116719: <http://trac.webkit.org/changeset/116719>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.