WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
updated patch for landing
(38.53 KB, patch)
2012-05-10 11:44 PDT
,
Alexey Proskuryakov
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
updated to ToT
(38.56 KB, patch)
2012-05-10 13:40 PDT
,
Alexey Proskuryakov
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
with chromium typo fix
(38.56 KB, patch)
2012-05-10 15:35 PDT
,
Alexey Proskuryakov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug