101211 2012-11-05 06:05:01 -0800 Protect against resource deletion during iteration in MemoryCache::pruneDeadResourcesToSize 2013-04-11 08:51:25 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 koivisto webkit-unassigned ap beidson jamesr japhet kling koivisto mrahaman vangelis webkit.review.bot oldest_to_newest 758464 0 koivisto 2012-11-05 06:05:01 -0800 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. 758465 1 koivisto 2012-11-05 06:05:26 -0800 <rdar://problem/11615488> 758475 2 172325 koivisto 2012-11-05 06:20:23 -0800 Created attachment 172325 patch 758489 3 172325 kling 2012-11-05 06:39:01 -0800 Comment on attachment 172325 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' 758497 4 koivisto 2012-11-05 07:03:02 -0800 http://trac.webkit.org/changeset/133469 788613 5 jamesr 2012-12-11 11:06:23 -0800 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? 788622 6 koivisto 2012-12-11 11:20:51 -0800 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. 789035 7 vangelis 2012-12-11 17:31:54 -0800 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. 873604 8 mrahaman 2013-04-11 00:57:12 -0700 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.. 873840 9 ap 2013-04-11 08:51:25 -0700 We do not comment about products based on WebKit. 172325 2012-11-05 06:20:23 -0800 2012-11-05 06:39:00 -0800 patch prune-protect-2.patch text/plain 3941 koivisto SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMzQ2MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDEyLTExLTA1ICBBbnR0aSBL b2l2aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBQcm90ZWN0IGFnYWluc3QgcmVz b3VyY2UgZGVsZXRpb24gZHVyaW5nIGl0ZXJhdGlvbiBpbiBNZW1vcnlDYWNoZTo6cHJ1bmVEZWFk UmVzb3VyY2VzVG9TaXplCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xMDEyMTEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAg ICAgICAgCisgICAgICAgIFNvbWUgY3Jhc2hlcyBoYXZlIGJlZW4gc2VlbiB1bmRlciBNZW1vcnlD YWNoZTo6cHJ1bmVEZWFkUmVzb3VyY2VzVG9TaXplLiBBIHBvc3NpYmxlIGNhdXNlIGlzIHRoYXQK KyAgICAgICAgZGVzdHJveURlY29kZWREYXRhKCkgY2FsbCBlbmRzIHVwIGV2aWN0aW5nIHRoZSBy ZXNvdXJjZSBwb2ludGVkIGJ5ICdwcmV2aW91cycgcG9pbnRlciBkdXJpbmcgaXRlcmF0aW9uCisg ICAgICAgIGFuZCBkZWxldGluZyB0aGUgb2JqZWN0LiBUaGlzIGxvb2tzIGluIHByaW5jaXBsZSBw b3NzaWJsZSB2aWEgc3R5bGVzaGVldHMgYW5kIFNWRyBpbWFnZXMuCisgICAgICAgIAorICAgICAg ICBTcGVjdWxhdGl2ZSBmaXgsIG5vIHJlcHJvLCBubyBvYnZpb3VzIHdheSB0byBjb25zdHJ1Y3Qg YSB0ZXN0LgorCisgICAgICAgICogbG9hZGVyL2NhY2hlL01lbW9yeUNhY2hlLmNwcDoKKyAgICAg ICAgKFdlYkNvcmU6Ok1lbW9yeUNhY2hlOjpwcnVuZURlYWRSZXNvdXJjZXNUb1NpemUpOgorICAg ICAgICAKKyAgICAgICAgICAgIFVzZSBDYWNoZWRSZXNvdXJjZUhhbmRsZSB0byBwcm90ZWN0IHRo ZSAncHJldmlvdXMnIHBvaW50ZXIgZHVyaW5nIGl0ZXJhdGlvbi4gQ2hlY2sgaWYgdGhlCisgICAg ICAgICAgICByZXNvdXJjZSBoYXMgYmVlbiBraWNrZWQgb3V0IGZyb20gdGhlIGNhY2hlIGR1cmlu ZyBkZXN0cm95RGVjb2RlZERhdGEoKSBhbmQgc3RvcCBpdGVyYXRpbmcKKyAgICAgICAgICAgIGlm IGhhcyAoYXMgaXQgbWF5IGRpZSB3aGVuIENhY2hlZFJlc291cmNlSGFuZGxlIHJlbGVhc2VzIGl0 KS4KKyAgICAgICAgICAgIFRoZSAnY3VycmVudCcgcG9pbnRlciBpcyBub3QgcHJvdGVjdGVkIGFz IHRoZSByZXNvdXJjZSBpdCBwb2ludHMgdG8gaXMgYWxsb3dlZCB0byBkaWUuCisgICAgICAgICAg ICAKIDIwMTItMTEtMDUgIFBhdmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBjaHJvbWl1bS5vcmc+CiAK ICAgICAgICAgV2ViIEluc3BlY3RvcjogcmVuZGVyIG1lc3NhZ2UgYnViYmxlcyBpbiBDb2RlTWly cm9yIGV4cGVyaW1lbnQuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvTWVtb3J5 Q2FjaGUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9NZW1v cnlDYWNoZS5jcHAJKHJldmlzaW9uIDEzMzIxMykKKysrIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9j YWNoZS9NZW1vcnlDYWNoZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTMwOCwyMyArMzA4LDMwIEBA IHZvaWQgTWVtb3J5Q2FjaGU6OnBydW5lRGVhZFJlc291cmNlc1RvU2kKICAgICAgICAgCiAgICAg ICAgIC8vIEZpcnN0IGZsdXNoIGFsbCB0aGUgZGVjb2RlZCBkYXRhIGluIHRoaXMgcXVldWUuCiAg ICAgICAgIHdoaWxlIChjdXJyZW50KSB7Ci0gICAgICAgICAgICBDYWNoZWRSZXNvdXJjZSogcHJl diA9IGN1cnJlbnQtPm1fcHJldkluQWxsUmVzb3VyY2VzTGlzdDsKKyAgICAgICAgICAgIC8vIFBy b3RlY3QgcHJldiBzbyBpdCBjYW4ndCBnZXQgZGVsZXRlZCBkdXJpbmcgZGVzdHJveURlY29kZWRE YXRhKCkuCisgICAgICAgICAgICBDYWNoZWRSZXNvdXJjZUhhbmRsZTxDYWNoZWRSZXNvdXJjZT4g cHJldmlvdXMgPSBjdXJyZW50LT5tX3ByZXZJbkFsbFJlc291cmNlc0xpc3Q7CisgICAgICAgICAg ICBBU1NFUlQoIXByZXZpb3VzIHx8IHByZXZpb3VzLT5pbkNhY2hlKCkpOwogICAgICAgICAgICAg aWYgKCFjdXJyZW50LT5oYXNDbGllbnRzKCkgJiYgIWN1cnJlbnQtPmlzUHJlbG9hZGVkKCkgJiYg Y3VycmVudC0+aXNMb2FkZWQoKSkgewogICAgICAgICAgICAgICAgIC8vIERlc3Ryb3kgb3VyIGRl Y29kZWQgZGF0YS4gVGhpcyB3aWxsIHJlbW92ZSB1cyBmcm9tIAogICAgICAgICAgICAgICAgIC8v IG1fbGl2ZURlY29kZWRSZXNvdXJjZXMsIGFuZCBwb3NzaWJseSBtb3ZlIHVzIHRvIGEgZGlmZmVy ZW50IAogICAgICAgICAgICAgICAgIC8vIExSVSBsaXN0IGluIG1fYWxsUmVzb3VyY2VzLgogICAg ICAgICAgICAgICAgIGN1cnJlbnQtPmRlc3Ryb3lEZWNvZGVkRGF0YSgpOwotICAgICAgICAgICAg ICAgIAorCiAgICAgICAgICAgICAgICAgaWYgKHRhcmdldFNpemUgJiYgbV9kZWFkU2l6ZSA8PSB0 YXJnZXRTaXplKQogICAgICAgICAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgICAgICB9Ci0g ICAgICAgICAgICBjdXJyZW50ID0gcHJldjsKKyAgICAgICAgICAgIC8vIERlY29kZWQgZGF0YSBt YXkgcmVmZXJlbmNlIG90aGVyIHJlc291cmNlcy4gU3RvcCBpdGVyYXRpbmcgaWYgJ3ByZXZpb3Vz JyBzb21laG93IGdvdAorICAgICAgICAgICAgLy8ga2lja2VkIG91dCBvZiBjYWNoZSBkdXJpbmcg ZGVzdHJveURlY29kZWREYXRhKCkuCisgICAgICAgICAgICBpZiAocHJldmlvdXMgJiYgIXByZXZp b3VzLT5pbkNhY2hlKCkpCisgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAgICAgICBjdXJy ZW50ID0gcHJldmlvdXMuZ2V0KCk7CiAgICAgICAgIH0KIAogICAgICAgICAvLyBOb3cgZXZpY3Qg b2JqZWN0cyBmcm9tIHRoaXMgcXVldWUuCiAgICAgICAgIGN1cnJlbnQgPSBtX2FsbFJlc291cmNl c1tpXS5tX3RhaWw7CiAgICAgICAgIHdoaWxlIChjdXJyZW50KSB7Ci0gICAgICAgICAgICBDYWNo ZWRSZXNvdXJjZSogcHJldiA9IGN1cnJlbnQtPm1fcHJldkluQWxsUmVzb3VyY2VzTGlzdDsKKyAg ICAgICAgICAgIENhY2hlZFJlc291cmNlSGFuZGxlPENhY2hlZFJlc291cmNlPiBwcmV2aW91cyA9 IGN1cnJlbnQtPm1fcHJldkluQWxsUmVzb3VyY2VzTGlzdDsKKyAgICAgICAgICAgIEFTU0VSVCgh cHJldmlvdXMgfHwgcHJldmlvdXMtPmluQ2FjaGUoKSk7CiAgICAgICAgICAgICBpZiAoIWN1cnJl bnQtPmhhc0NsaWVudHMoKSAmJiAhY3VycmVudC0+aXNQcmVsb2FkZWQoKSAmJiAhY3VycmVudC0+ aXNDYWNoZVZhbGlkYXRvcigpKSB7CiAgICAgICAgICAgICAgICAgaWYgKCFtYWtlUmVzb3VyY2VQ dXJnZWFibGUoY3VycmVudCkpCiAgICAgICAgICAgICAgICAgICAgIGV2aWN0KGN1cnJlbnQpOwpA QCAtMzMyLDcgKzMzOSw5IEBAIHZvaWQgTWVtb3J5Q2FjaGU6OnBydW5lRGVhZFJlc291cmNlc1Rv U2kKICAgICAgICAgICAgICAgICBpZiAodGFyZ2V0U2l6ZSAmJiBtX2RlYWRTaXplIDw9IHRhcmdl dFNpemUpCiAgICAgICAgICAgICAgICAgICAgIHJldHVybjsKICAgICAgICAgICAgIH0KLSAgICAg ICAgICAgIGN1cnJlbnQgPSBwcmV2OworICAgICAgICAgICAgaWYgKHByZXZpb3VzICYmICFwcmV2 aW91cy0+aW5DYWNoZSgpKQorICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgY3Vy cmVudCA9IHByZXZpb3VzLmdldCgpOwogICAgICAgICB9CiAgICAgICAgICAgICAKICAgICAgICAg Ly8gU2hyaW5rIHRoZSB2ZWN0b3IgYmFjayBkb3duIHNvIHdlIGRvbid0IHdhc3RlIHRpbWUgaW5z cGVjdGluZwo=