RESOLVED FIXED 86027
Crash in 3rd party WebKit apps that disable cache at a wrong time
https://bugs.webkit.org/show_bug.cgi?id=86027
Summary Crash in 3rd party WebKit apps that disable cache at a wrong time
Alexey Proskuryakov
Reported 2012-05-09 15:53:52 PDT
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>
Attachments
proposed fix (37.14 KB, patch)
2012-05-09 16:03 PDT, Alexey Proskuryakov
koivisto: review+
webkit.review.bot: commit-queue-
updated patch for landing (38.53 KB, patch)
2012-05-10 11:44 PDT, Alexey Proskuryakov
webkit.review.bot: commit-queue-
updated to ToT (38.56 KB, patch)
2012-05-10 13:40 PDT, Alexey Proskuryakov
webkit.review.bot: commit-queue-
with chromium typo fix (38.56 KB, patch)
2012-05-10 15:35 PDT, Alexey Proskuryakov
no flags
Alexey Proskuryakov
Comment 1 2012-05-09 16:03:40 PDT
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 2 2012-05-09 16:21:28 PDT
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 3 2012-05-09 16:54:18 PDT
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 4 2012-05-10 02:26:05 PDT
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
Comment 5 2012-05-10 11:24:47 PDT
- 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
Comment 6 2012-05-10 11:44:22 PDT
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 7 2012-05-10 12:12:03 PDT
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
Comment 8 2012-05-10 13:40:43 PDT
Created attachment 141241 [details] updated to ToT
WebKit Review Bot
Comment 9 2012-05-10 15:33:37 PDT
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
Comment 10 2012-05-10 15:35:36 PDT
Created attachment 141280 [details] with chromium typo fix
WebKit Review Bot
Comment 11 2012-05-10 19:30:43 PDT
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
Comment 12 2012-05-10 19:30:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.