Bug 101211

Summary: Protect against resource deletion during iteration in MemoryCache::pruneDeadResourcesToSize
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, jamesr, japhet, kling, koivisto, mrahaman, vangelis, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch kling: review+

Description Antti Koivisto 2012-11-05 06:05:01 PST
There have been some crashes that look like this:

 1                                0x000000000000003f 0 + 63
  2 com.apple.WebCore              0x7fff86c26b47 WebCore::MemoryCache::pruneDeadResourcesToSize(unsigned int) + 0x1f7
   3 com.apple.WebCore              0x7fff86ba8507 WebCore::MemoryCache::prune() + 0x67
   4 com.apple.WebCore              0x7fff8733cbe6 WebCore::BitmapImage::draw(WebCore::GraphicsContext*, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ColorSpace, WebCore::CompositeOperator, WebCore::RespectImageOrientationEnum) + 0xf6
   5 com.apple.WebCore              0x7fff86ccc364 WebCore::BitmapImage::draw(WebCore::GraphicsContext*, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ColorSpace, WebCore::CompositeOperator) + 0x14
   6 com.apple.WebCore              0x7fff86d50297 WebCore::Image::drawTiled(WebCore::GraphicsContext*, WebCore::FloatRect const&, WebCore::FloatPoint const&, WebCore::FloatSize const&, WebCore::ColorSpace, WebCore::CompositeOperator) + 0x277
   7 com.apple.WebCore              0x7fff86d50011 

A possible cause is that call to destroyDecodedData() causes other resources besides the current one to be evicted from cache.
Comment 1 Antti Koivisto 2012-11-05 06:05:26 PST
<rdar://problem/11615488>
Comment 2 Antti Koivisto 2012-11-05 06:20:23 PST
Created attachment 172325 [details]
patch
Comment 3 Andreas Kling 2012-11-05 06:39:01 PST
Comment on attachment 172325 [details]
patch

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

Looks safe and mostly reasonable. r=me

> Source/WebCore/loader/cache/MemoryCache.cpp:311
> +            // Protect prev so it can't get deleted during destroyDecodedData().

prev -> 'previous'
Comment 4 Antti Koivisto 2012-11-05 07:03:02 PST
http://trac.webkit.org/changeset/133469
Comment 5 James Robinson 2012-12-11 11:06:23 PST
This appears to have changed the eviction behavior on some of our performance tests, e.g. the spike here:

http://build.chromium.org/f/chromium/perf/gpu-mac-release-intel/gpu_throughput/report.html?rev=166239&graph=many_images&trace=software&history=150

Is that expected?
Comment 6 Antti Koivisto 2012-12-11 11:20:51 PST
The crashing/memory corruption case turns into a case where we interrupt the eviction in the middle, so this can affect eviction behavior. This is not ideal but it was the safest and simplest fix I could think of.
Comment 7 Vangelis Kokkevis 2012-12-11 17:31:54 PST
Antti,

This looks like it introduced a significant perf regression in Chromium that points to additional calls to WebCore::CachedResource::registerHandle and WebCore::CachedResource::unregisterHandle

See:

http://code.google.com/p/chromium/issues/detail?id=159555

It may have similar perf implications to other parts of WebKit as well.
Comment 8 Mustafizur Rahaman( :rahaman) 2013-04-11 00:57:12 PDT
Can anyone please tell me if this fix is integrated in the UIWebView that we use in iPad3? I am seeing a similar crash in iPad3 & wanted to know if this patch is already used there? I tried to find in apple bug report (https://bugreport.apple.com) but could not find much details..
Comment 9 Alexey Proskuryakov 2013-04-11 08:51:25 PDT
We do not comment about products based on WebKit.