28473 2009-08-19 14:42:07 -0700 Crashes on sites with lots of images 2009-08-20 22:57:40 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 levin levin ap dglazkov koivisto oldest_to_newest 141606 0 levin 2009-08-19 14:42:07 -0700 This occurs as a result of http://trac.webkit.org/changeset/42896 The problem is that m_resourceToRevalidate::m_isBeingRevalidated is false while CachedResource is still referring to it, so it may get deleted before Cache::revalidationSucceeded calls CachedResource::clearResourceToRevalidate. Specially, m_resourceToRevalidate::didAddClient may result in a call to m_resourceToRevalidate::removeClient which may call Cache::prune and delete m_resourceToRevalidate. Here's a typical call stack for the crash in chromium: WebCore::CachedResource::clearResourceToRevalidate() Line 313 WebCore::Cache::revalidationSucceeded WebCore::Loader::Host::didReceiveResponse WebCore::SubresourceLoader::didReceiveResponse WebCore::ResourceLoader::didReceiveResponse WebCore::ResourceHandleInternal::didReceiveResponse Note that this requires a pretty full cache to repro. http://code.google.com/p/chromium/issues/detail?id=15375 141608 1 levin 2009-08-19 14:52:40 -0700 One site that seemed to expose this is http://madbadger.net/index.php 141610 2 35148 levin 2009-08-19 15:00:53 -0700 Created attachment 35148 Proposed fix. Free free to r- for lack of tests :) but I wanted to get feedback about the patch without that part. For the layout tests, here's what I'm thinking about (approximately and it will take a bit of tweaking for me to hit these conditions precisely): Add a new cache model (or see if I can do it with existing models) and allow it to be selected from LayoutTestController. Load a bunch of images to hit the boundary and expose this issue. Feel free to give feedback on that approach as well. 141624 3 35148 darin 2009-08-19 15:54:02 -0700 Comment on attachment 35148 Proposed fix. > + better reflect its expanded roll. role This patch looks good to me. Antti may have some thoughts. He's sort of an expert on the cache/loader. 141628 4 dglazkov 2009-08-19 16:06:46 -0700 Dude, I can't believe you figured this one out. 141648 5 35148 ap 2009-08-19 17:12:19 -0700 Comment on attachment 35148 Proposed fix. + better reflect its expanded roll. role I think the patch is very good. The only concern I have is that using CachedResourceHandle just as a protector is abuse - we discussed some other approaches on IRC, maybe they will lead to a better solution. Also, making cache testable may be out of scope for this patch. But that's something we sorely lack, for sure. 141650 6 35148 levin 2009-08-19 17:13:15 -0700 Comment on attachment 35148 Proposed fix. I'll try to find another way to do this without abusing CachedResourceHandle. A few ideas mentioned by ap: * move switchToRevalidatedResource() before the original resource back to cache * there are several checks in pruneDeadResources() already, maybe we could add another check there? * zero out m_resourceToRevalidate at the time it's deleted using a parent pointer. 141729 7 koivisto 2009-08-19 21:09:46 -0700 What Alexey said. Looks great except for the questionable use of CachedResourceHandle which shouldn't be used as a stack variable (it would be nice to turn CachedResources to real refcounted class though). 141815 8 ap 2009-08-20 22:57:40 -0700 Bugzilla has experienced amnesia today. This bug was fixed by Dave in <http://trac.webkit.org/changeset/47602>. 35148 2009-08-19 15:00:53 -0700 2009-08-19 17:13:14 -0700 Proposed fix. bug28473.txt text/plain 5734 levin ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg OGE4YzZjNS4uNzk4ODA2MiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwzNCBAQAorMjAwOS0wOC0xOSAgRGF2aWQgTGV2aW4g IDxsZXZpbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQ3Jhc2hlcyBvbiBzaXRlcyB3aXRoIGxvdHMgb2YgaW1hZ2VzLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Mjg0NzMKKworICAg ICAgICBUaGUgcHJvYmxlbSBpcyB0aGF0IG1fcmVzb3VyY2VUb1JldmFsaWRhdGU6Om1faXNCZWlu Z1JldmFsaWRhdGVkIGlzIGZhbHNlIHdoaWxlCisgICAgICAgIENhY2hlZFJlc291cmNlIGlzIHN0 aWxsIHJlZmVycmluZyB0byBpdCwgc28gaXQgbWF5IGdldCBkZWxldGVkIGJlZm9yZQorICAgICAg ICBDYWNoZTo6cmV2YWxpZGF0aW9uU3VjY2VlZGVkIGNhbGxzIENhY2hlZFJlc291cmNlOjpjbGVh clJlc291cmNlVG9SZXZhbGlkYXRlLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gKE9PUFMhKQor CisgICAgICAgICogbG9hZGVyL0NhY2hlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhY2hlOjpy ZXZhbGlkYXRlUmVzb3VyY2UpOiBBZGRlZCBhc3NlcnQgdG8gdmVyaWZ5IHRoZSByZXNvdXJjZQor ICAgICAgICBiZWluZyByZXZhbGlkYXRlZCBpcyBwaHlzaWNhbGx5IGluIHRoZSBjYWNoZS4KKyAg ICAgICAgKFdlYkNvcmU6OkNhY2hlOjpyZXZhbGlkYXRpb25TdWNjZWVkZWQpOiBDaGFuZ2VkIHRv IGRvIG9uZSBjYWxsIHRvIHN3aXRjaCBmcm9tIHRoZSByZXZhbGlkYXRpbmcKKyAgICAgICAgcmVz b3VyY2UgdG8gdGhlIHJldmFsaWRhdGVkIHJlc291cmNlLiAgVGhpcyBhbGxvd3MgZm9yIHByb3Bl ciBsaWZldGltZSBtYW5hZ2VtZW50IG9mIHRoZQorICAgICAgICByZXZhbGlkYXRlZCByZXNvdXJj ZS4gQWRkZWQgYXNzZXJ0IHRvIHZlcmlmeSB0aGF0IHRoZSByZXZhbGlkYXRpbmdSZXNvdXJjZSBp cyBpbiB0aGUgY2FjaGUuCisgICAgICAgIChXZWJDb3JlOjpDYWNoZTo6cHJ1bmVEZWFkUmVzb3Vy Y2VzKTogRG9uJ3QgcmVtb3ZlIGl0ZW1zIGZyb20gdGhlIGNhY2hlIHRoYXQgYXJlIGluIHRoZQor ICAgICAgICBwcm9jZXNzIG9mIGJlaW5nIHZhbGlkYXRlZCAob3IgZWxzZSB0aGV5IGNhbiBnZXQg YWRkZWQgdHdpY2U6IE9uY2UgdGhyb3VnaCB0aGUgbm9ybWFsIG1lY2hhbmlzbQorICAgICAgICBh bmQgdGhlbiBhZ2FpbiBkdXJpbmcgcmV2YWxpZGF0ZVJlc291cmNlKS4KKworICAgICAgICAqIGxv YWRlci9DYWNoZWRSZXNvdXJjZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRSZXNvdXJj ZTo6c3dpdGNoVG9SZXZhbGlkYXRlZFJlc291cmNlKTogUHJldmVudCB0aGUgcmVzb3VyY2UgdGhh dCB3YXMgYmVpbmcKKyAgICAgICAgcmV2YWxpZGF0ZWQgZnJvbSBnZXR0aW5nIGRlbGV0ZWQgd2hp bGUgdGhpcyBtZXRob2QgaXMgYmVpbmcgcnVuLgorCisgICAgICAgICogbG9hZGVyL0NhY2hlZFJl c291cmNlLmg6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRSZXNvdXJjZTo6c3dpdGNoVG9SZXZh bGlkYXRlZFJlc291cmNlKTogQ2hhbmdlZCB0aGUgbmFtZSBvZiB0aGlzIG1ldGhvZCB0bworICAg ICAgICBiZXR0ZXIgcmVmbGVjdCBpdHMgZXhwYW5kZWQgcm9sbC4KKwogMjAwOS0wOC0xOCAgRHJl dyBXaWxzb24gIDxhdHdpbHNvbkBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERh dmlkIExldmluLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9sb2FkZXIvQ2FjaGUuY3BwIGIvV2ViQ29y ZS9sb2FkZXIvQ2FjaGUuY3BwCmluZGV4IDI1MGVhMGQuLjU4OWI2MDkgMTAwNjQ0Ci0tLSBhL1dl YkNvcmUvbG9hZGVyL0NhY2hlLmNwcAorKysgYi9XZWJDb3JlL2xvYWRlci9DYWNoZS5jcHAKQEAg LTE4Miw2ICsxODIsNyBAQCB2b2lkIENhY2hlOjpyZXZhbGlkYXRlUmVzb3VyY2UoQ2FjaGVkUmVz b3VyY2UqIHJlc291cmNlLCBEb2NMb2FkZXIqIGRvY0xvYWRlcikKIHsKICAgICBBU1NFUlQocmVz b3VyY2UpOwogICAgIEFTU0VSVChyZXNvdXJjZS0+aW5DYWNoZSgpKTsKKyAgICBBU1NFUlQocmVz b3VyY2UgPT0gbV9yZXNvdXJjZXMuZ2V0KHJlc291cmNlLT51cmwoKSkpOwogICAgIEFTU0VSVCgh ZGlzYWJsZWQoKSk7CiAgICAgaWYgKHJlc291cmNlLT5yZXNvdXJjZVRvUmV2YWxpZGF0ZSgpKQog ICAgICAgICByZXR1cm47CkBAIC0yMDUsNiArMjA2LDcgQEAgdm9pZCBDYWNoZTo6cmV2YWxpZGF0 aW9uU3VjY2VlZGVkKENhY2hlZFJlc291cmNlKiByZXZhbGlkYXRpbmdSZXNvdXJjZSwgY29uc3Qg UmUKICAgICBBU1NFUlQocmVzb3VyY2UpOwogICAgIEFTU0VSVCghcmVzb3VyY2UtPmluQ2FjaGUo KSk7CiAgICAgQVNTRVJUKHJlc291cmNlLT5pc0xvYWRlZCgpKTsKKyAgICBBU1NFUlQocmV2YWxp ZGF0aW5nUmVzb3VyY2UtPmluQ2FjaGUoKSk7CiAgICAgCiAgICAgZXZpY3QocmV2YWxpZGF0aW5n UmVzb3VyY2UpOwogCkBAIC0yMTksOSArMjIxLDggQEAgdm9pZCBDYWNoZTo6cmV2YWxpZGF0aW9u U3VjY2VlZGVkKENhY2hlZFJlc291cmNlKiByZXZhbGlkYXRpbmdSZXNvdXJjZSwgY29uc3QgUmUK ICAgICBpZiAoZGVsdGEpCiAgICAgICAgIGFkanVzdFNpemUocmVzb3VyY2UtPmhhc0NsaWVudHMo KSwgZGVsdGEpOwogICAgIAotICAgIHJldmFsaWRhdGluZ1Jlc291cmNlLT5zd2l0Y2hDbGllbnRz VG9SZXZhbGlkYXRlZFJlc291cmNlKCk7Ci0gICAgLy8gdGhpcyBkZWxldGVzIHRoZSByZXZhbGlk YXRpbmcgcmVzb3VyY2UKLSAgICByZXZhbGlkYXRpbmdSZXNvdXJjZS0+Y2xlYXJSZXNvdXJjZVRv UmV2YWxpZGF0ZSgpOworICAgIC8vIFRoaXMgZGVsZXRlcyB0aGUgcmV2YWxpZGF0aW5nIHJlc291 cmNlLgorICAgIHJldmFsaWRhdGluZ1Jlc291cmNlLT5zd2l0Y2hUb1JldmFsaWRhdGVkUmVzb3Vy Y2UoKTsKIH0KIAogdm9pZCBDYWNoZTo6cmV2YWxpZGF0aW9uRmFpbGVkKENhY2hlZFJlc291cmNl KiByZXZhbGlkYXRpbmdSZXNvdXJjZSkKQEAgLTM1MSw3ICszNTIsNyBAQCB2b2lkIENhY2hlOjpw cnVuZURlYWRSZXNvdXJjZXMoKQogICAgICAgICBjdXJyZW50ID0gbV9hbGxSZXNvdXJjZXNbaV0u bV90YWlsOwogICAgICAgICB3aGlsZSAoY3VycmVudCkgewogICAgICAgICAgICAgQ2FjaGVkUmVz b3VyY2UqIHByZXYgPSBjdXJyZW50LT5tX3ByZXZJbkFsbFJlc291cmNlc0xpc3Q7Ci0gICAgICAg ICAgICBpZiAoIWN1cnJlbnQtPmhhc0NsaWVudHMoKSAmJiAhY3VycmVudC0+aXNQcmVsb2FkZWQo KSkgeworICAgICAgICAgICAgaWYgKCFjdXJyZW50LT5oYXNDbGllbnRzKCkgJiYgIWN1cnJlbnQt PmlzUHJlbG9hZGVkKCkgJiYgIWN1cnJlbnQtPmlzQ2FjaGVWYWxpZGF0b3IoKSkgewogICAgICAg ICAgICAgICAgIGV2aWN0KGN1cnJlbnQpOwogICAgICAgICAgICAgICAgIC8vIElmIGV2aWN0KCkg Y2F1c2VkIHBydW5lRGVhZFJlc291cmNlcygpIHRvIGJlIHJlLWVudGVyZWQsIGJhaWwgb3V0LiBU aGlzIGNhbiBoYXBwZW4gd2hlbiByZW1vdmluZyBhbgogICAgICAgICAgICAgICAgIC8vIFNWRyBD YWNoZWRJbWFnZSB0aGF0IGhhcyBzdWJyZXNvdXJjZXMuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2xv YWRlci9DYWNoZWRSZXNvdXJjZS5jcHAgYi9XZWJDb3JlL2xvYWRlci9DYWNoZWRSZXNvdXJjZS5j cHAKaW5kZXggNDc4ODczZi4uMjk3ZDEyYyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9sb2FkZXIvQ2Fj aGVkUmVzb3VyY2UuY3BwCisrKyBiL1dlYkNvcmUvbG9hZGVyL0NhY2hlZFJlc291cmNlLmNwcApA QCAtMzE1LDEyICszMTUsMTcgQEAgdm9pZCBDYWNoZWRSZXNvdXJjZTo6Y2xlYXJSZXNvdXJjZVRv UmV2YWxpZGF0ZSgpCiAgICAgZGVsZXRlSWZQb3NzaWJsZSgpOwogfQogICAgIAotdm9pZCBDYWNo ZWRSZXNvdXJjZTo6c3dpdGNoQ2xpZW50c1RvUmV2YWxpZGF0ZWRSZXNvdXJjZSgpCit2b2lkIENh Y2hlZFJlc291cmNlOjpzd2l0Y2hUb1JldmFsaWRhdGVkUmVzb3VyY2UoKQogewogICAgIEFTU0VS VChtX3Jlc291cmNlVG9SZXZhbGlkYXRlKTsKICAgICBBU1NFUlQobV9yZXNvdXJjZVRvUmV2YWxp ZGF0ZS0+aW5DYWNoZSgpKTsKKyAgICBBU1NFUlQoIW1fcmVzb3VyY2VUb1JldmFsaWRhdGUtPm1f aXNCZWluZ1JldmFsaWRhdGVkKTsKICAgICBBU1NFUlQoIWluQ2FjaGUoKSk7CiAKKyAgICAvLyBQ cmV2ZW50IG1fcmVzb3VyY2VUb1JldmFsaWRhdGUgZnJvbSBiZWluZyBkZWxldGVkIGluIGhlcmUg d2hpY2ggbWF5IGhhcHBlbgorICAgIC8vIGJlY2F1c2UgZGlkQWRkQ2xpZW50IG1heSByZXN1bHQg aW4gYSBjYWxsIHRvIHJlbW92ZUNsaWVudCB3aGljaCBkb2VzIGEgQ2FjaGU6OlBydW5lLgorICAg IENhY2hlZFJlc291cmNlSGFuZGxlPENhY2hlZFJlc291cmNlPiBwcm90ZWN0KG1fcmVzb3VyY2VU b1JldmFsaWRhdGUpOworCiAgICAgSGFzaFNldDxDYWNoZWRSZXNvdXJjZUhhbmRsZUJhc2UqPjo6 aXRlcmF0b3IgZW5kID0gbV9oYW5kbGVzVG9SZXZhbGlkYXRlLmVuZCgpOwogICAgIGZvciAoSGFz aFNldDxDYWNoZWRSZXNvdXJjZUhhbmRsZUJhc2UqPjo6aXRlcmF0b3IgaXQgPSBtX2hhbmRsZXNU b1JldmFsaWRhdGUuYmVnaW4oKTsgaXQgIT0gZW5kOyArK2l0KSB7CiAgICAgICAgIENhY2hlZFJl c291cmNlSGFuZGxlQmFzZSogaGFuZGxlID0gKml0OwpAQCAtMzUyLDYgKzM1Nyw4IEBAIHZvaWQg Q2FjaGVkUmVzb3VyY2U6OnN3aXRjaENsaWVudHNUb1JldmFsaWRhdGVkUmVzb3VyY2UoKQogICAg ICAgICBpZiAobV9yZXNvdXJjZVRvUmV2YWxpZGF0ZS0+bV9jbGllbnRzLmNvbnRhaW5zKGNsaWVu dHNUb01vdmVbbl0pKQogICAgICAgICAgICAgbV9yZXNvdXJjZVRvUmV2YWxpZGF0ZS0+ZGlkQWRk Q2xpZW50KGNsaWVudHNUb01vdmVbbl0pOwogICAgIH0KKyAgICBtX3Jlc291cmNlVG9SZXZhbGlk YXRlID0gMDsKKyAgICBkZWxldGVJZlBvc3NpYmxlKCk7CiB9CiAgICAgCiB2b2lkIENhY2hlZFJl c291cmNlOjp1cGRhdGVSZXNwb25zZUFmdGVyUmV2YWxpZGF0aW9uKGNvbnN0IFJlc291cmNlUmVz cG9uc2UmIHZhbGlkYXRpbmdSZXNwb25zZSkKZGlmZiAtLWdpdCBhL1dlYkNvcmUvbG9hZGVyL0Nh Y2hlZFJlc291cmNlLmggYi9XZWJDb3JlL2xvYWRlci9DYWNoZWRSZXNvdXJjZS5oCmluZGV4IGJh YmRmODkuLmU1YzYxYmIgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvbG9hZGVyL0NhY2hlZFJlc291cmNl LmgKKysrIGIvV2ViQ29yZS9sb2FkZXIvQ2FjaGVkUmVzb3VyY2UuaApAQCAtMjA5LDcgKzIwOSw3 IEBAIHByaXZhdGU6CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCiAg ICAgLy8gVGhlc2UgYXJlIGNhbGxlZCBieSB0aGUgZnJpZW5kbHkgQ2FjaGUgb25seQogICAgIHZv aWQgc2V0UmVzb3VyY2VUb1JldmFsaWRhdGUoQ2FjaGVkUmVzb3VyY2UqKTsKLSAgICB2b2lkIHN3 aXRjaENsaWVudHNUb1JldmFsaWRhdGVkUmVzb3VyY2UoKTsKKyAgICB2b2lkIHN3aXRjaFRvUmV2 YWxpZGF0ZWRSZXNvdXJjZSgpOwogICAgIHZvaWQgY2xlYXJSZXNvdXJjZVRvUmV2YWxpZGF0ZSgp OwogICAgIHZvaWQgdXBkYXRlUmVzcG9uc2VBZnRlclJldmFsaWRhdGlvbihjb25zdCBSZXNvdXJj ZVJlc3BvbnNlJiB2YWxpZGF0aW5nUmVzcG9uc2UpOwogCg==