Bug 101211 – Protect against resource deletion during iteration in MemoryCache::pruneDeadResourcesToSize

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

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.

[Antti Koivisto]

<rdar://problem/11615488>

[Antti Koivisto]

Created attachment 172325 [details]
patch

[Andreas Kling]

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'

[Antti Koivisto]

http://trac.webkit.org/changeset/133469

[James Robinson]

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?

[Antti Koivisto]

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.

[Vangelis Kokkevis]

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.

[Mustafizur Rahaman( :rahaman)]

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..

[Alexey Proskuryakov]

We do not comment about products based on WebKit.