23180 2009-01-08 05:32:58 -0800 Reading freed memory at DocumentLoader::checkForPendingPreloads 2009-02-04 16:42:12 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Major --- 1 ager mad koivisto oldest_to_newest 104865 0 ager 2009-01-08 05:32:58 -0800 In loader.cpp in Loader::Host::didFinishLoading the following two lines causes the problem: docLoader->setLoadInProgress(false); docLoader->checkForPendingPreloads(); setLoadInProgress(false) can cause the frame to be deallocated, which can cause the document to be deallocated and the document destructor deletes the DocumentLoader. Therefore, docLoader can be freed memory at the call to checkForPendingPreloads. This happens when running the layout test: LayoutTests/http/tests/misc/onload-remove-iframe-crash-2.html. Would it make sense to just reorder and check for pending preloads before setting load in progress to false? 105028 1 koivisto 2009-01-09 11:17:00 -0800 Sounds sensible 107078 2 jon 2009-01-26 14:12:56 -0800 See http://code.google.com/p/chromium/issues/detail?id=3949 107692 3 mad 2009-01-30 06:52:14 -0800 Another solution I tried while working on the Chromium version of this bug is to hold on to a reference of the document while we interact with the docLoader... This would make the code safer for future maintenance and would not depend on the call order... I have a patch with that change (which is needed at two places by the way), that I will send for code review soon (once I know how to do this here, first time I try :-). (In reply to comment #0) > In loader.cpp in Loader::Host::didFinishLoading the following two lines causes > the problem: > > docLoader->setLoadInProgress(false); > docLoader->checkForPendingPreloads(); > > setLoadInProgress(false) can cause the frame to be deallocated, which can cause > the document to be deallocated and the document destructor deletes the > DocumentLoader. Therefore, docLoader can be freed memory at the call to > checkForPendingPreloads. > > This happens when running the layout test: > LayoutTests/http/tests/misc/onload-remove-iframe-crash-2.html. > > Would it make sense to just reorder and check for pending preloads before > setting load in progress to false? > 107805 4 27203 mad 2009-01-30 17:17:22 -0800 Created attachment 27203 Patch To fix the bug as proposed in bug comments from me (mad). 107830 5 koivisto 2009-01-31 02:15:21 -0800 Looks good, two comments: - I believe "protector" is the usual name for this kind of variables in WebKit. I don't know what "gard" means. - Please provide a ChangeLog entry. 107996 6 27252 mad 2009-02-02 12:18:43 -0800 Created attachment 27252 New version of the patch based on recent comments. Changed the variable name from gard (term used in Chromium, this is why I used it ;-), to protector which is more widely used in WebKit. Also added details to ChangeLog file (slowly getting used to the process :-) And fixed comments to be grammatically correct (start with a Capital and end with a period.). :-) 108257 7 27252 koivisto 2009-02-04 02:25:47 -0800 Comment on attachment 27252 New version of the patch based on recent comments. r=me 108368 8 eric 2009-02-04 16:42:12 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/loader/loader.cpp Committed r40648 27203 2009-01-30 17:17:22 -0800 2009-02-02 12:18:43 -0800 Patch Fix23180.txt text/plain 1645 mad SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MDQzMSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDktMDEtMzAgIG1hZCAgPHNldCBFTUFJTF9BRERSRVNTIGVudmly b25tZW50IHZhcmlhYmxlPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFdBUk5JTkc6IE5PIFRFU1QgQ0FTRVMgQURERUQgT1IgQ0hBTkdFRAorCisgICAg ICAgICogbG9hZGVyL2xvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpMb2FkZXI6Okhvc3Q6 OmRpZEZpbmlzaExvYWRpbmcpOgorICAgICAgICAoV2ViQ29yZTo6TG9hZGVyOjpIb3N0OjpkaWRG YWlsKToKKwogMjAwOS0wMS0zMCAgU2FtIFdlaW5pZyAgPHNhbUB3ZWJraXQub3JnPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IERhbiBCZXJuc3RlaW4uCkluZGV4OiBXZWJDb3JlL2xvYWRlci9sb2Fk ZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvbG9hZGVyL2xvYWRlci5jcHAJKHJldmlzaW9u IDQwNDA0KQorKysgV2ViQ29yZS9sb2FkZXIvbG9hZGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAt Mjg4LDYgKzI4OCw5IEBAIHZvaWQgTG9hZGVyOjpIb3N0OjpkaWRGaW5pc2hMb2FkaW5nKFN1YnIK ICAgICBSZXF1ZXN0KiByZXF1ZXN0ID0gaS0+c2Vjb25kOwogICAgIG1fcmVxdWVzdHNMb2FkaW5n LnJlbW92ZShpKTsKICAgICBEb2NMb2FkZXIqIGRvY0xvYWRlciA9IHJlcXVlc3QtPmRvY0xvYWRl cigpOworICAgIC8vIHByZXZlbnQgdGhlIGRvY3VtZW50IGZyb20gYmVpbmcgZGVzdHJveWVkIGJl Zm9yZSB3ZSBhcmUgZG9uZQorICAgIC8vIHdpdGggdGhlIGRvY0xvYWRlciB0aGF0IGl0IHdpbGwg ZGVsZXRlIHdoZW4gdGhlIGRvY3VtZW50IGdldHMgZGVsZXRlZAorICAgIERvY1B0cjxEb2N1bWVu dD4gZ2FyZChkb2NMb2FkZXItPmRvYygpKTsKICAgICBpZiAoIXJlcXVlc3QtPmlzTXVsdGlwYXJ0 KCkpCiAgICAgICAgIGRvY0xvYWRlci0+ZGVjcmVtZW50UmVxdWVzdENvdW50KCk7CiAKQEAgLTMz Myw2ICszMzYsOSBAQCB2b2lkIExvYWRlcjo6SG9zdDo6ZGlkRmFpbChTdWJyZXNvdXJjZUxvCiAg ICAgUmVxdWVzdCogcmVxdWVzdCA9IGktPnNlY29uZDsKICAgICBtX3JlcXVlc3RzTG9hZGluZy5y ZW1vdmUoaSk7CiAgICAgRG9jTG9hZGVyKiBkb2NMb2FkZXIgPSByZXF1ZXN0LT5kb2NMb2FkZXIo KTsKKyAgICAvLyBwcmV2ZW50IHRoZSBkb2N1bWVudCBmcm9tIGJlaW5nIGRlc3Ryb3llZCBiZWZv cmUgd2UgYXJlIGRvbmUKKyAgICAvLyB3aXRoIHRoZSBkb2NMb2FkZXIgdGhhdCBpdCB3aWxsIGRl bGV0ZSB3aGVuIHRoZSBkb2N1bWVudCBnZXRzIGRlbGV0ZWQKKyAgICBEb2NQdHI8RG9jdW1lbnQ+ IGdhcmQoZG9jTG9hZGVyLT5kb2MoKSk7CiAgICAgaWYgKCFyZXF1ZXN0LT5pc011bHRpcGFydCgp KQogICAgICAgICBkb2NMb2FkZXItPmRlY3JlbWVudFJlcXVlc3RDb3VudCgpOwogCg== 27252 2009-02-02 12:18:43 -0800 2009-02-04 02:25:47 -0800 New version of the patch based on recent comments. Fix23180.txt text/plain 1854 mad SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MDQ4NCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDItMDIgIG1hZCAgPG1hZEBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDIzMTgwIFJl YWRpbmcgZnJlZWQgbWVtb3J5IGF0IERvY3VtZW50TG9hZGVyOjpjaGVja0ZvclBlbmRpbmdQcmVs b2FkcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjMx ODAKKworICAgICAgICBBZGRlZCBhIHByb3RlY3RvciBmb3IgdGhlIGRvY3VtZW50IHBvdGVudGlh bGx5IGhvbGRpbmcgb24gdGhlIGxhc3QgcmVmZXJlbmNlIHRvIHRoZSBsb2FkZXIgd2UgYXJlIGlu dGVyYXRpbmcgd2l0aC4KKworICAgICAgICAqIGxvYWRlci9sb2FkZXIuY3BwOgorICAgICAgICAo V2ViQ29yZTo6TG9hZGVyOjpIb3N0OjpkaWRGaW5pc2hMb2FkaW5nKToKKyAgICAgICAgKFdlYkNv cmU6OkxvYWRlcjo6SG9zdDo6ZGlkRmFpbCk6CisKIDIwMDktMDItMDIgIERhcmluIEFkbGVyICA8 ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IE5pa28gWmltbWVybWFubi4K SW5kZXg6IFdlYkNvcmUvbG9hZGVyL2xvYWRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9s b2FkZXIvbG9hZGVyLmNwcAkocmV2aXNpb24gNDA0ODMpCisrKyBXZWJDb3JlL2xvYWRlci9sb2Fk ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yODgsNiArMjg4LDkgQEAgdm9pZCBMb2FkZXI6Okhv c3Q6OmRpZEZpbmlzaExvYWRpbmcoU3VicgogICAgIFJlcXVlc3QqIHJlcXVlc3QgPSBpLT5zZWNv bmQ7CiAgICAgbV9yZXF1ZXN0c0xvYWRpbmcucmVtb3ZlKGkpOwogICAgIERvY0xvYWRlciogZG9j TG9hZGVyID0gcmVxdWVzdC0+ZG9jTG9hZGVyKCk7CisgICAgLy8gUHJldmVudCB0aGUgZG9jdW1l bnQgZnJvbSBiZWluZyBkZXN0cm95ZWQgYmVmb3JlIHdlIGFyZSBkb25lIHdpdGgKKyAgICAvLyB0 aGUgZG9jTG9hZGVyIHRoYXQgaXQgd2lsbCBkZWxldGUgd2hlbiB0aGUgZG9jdW1lbnQgZ2V0cyBk ZWxldGVkLgorICAgIERvY1B0cjxEb2N1bWVudD4gcHJvdGVjdG9yKGRvY0xvYWRlci0+ZG9jKCkp OwogICAgIGlmICghcmVxdWVzdC0+aXNNdWx0aXBhcnQoKSkKICAgICAgICAgZG9jTG9hZGVyLT5k ZWNyZW1lbnRSZXF1ZXN0Q291bnQoKTsKIApAQCAtMzMzLDYgKzMzNiw5IEBAIHZvaWQgTG9hZGVy OjpIb3N0OjpkaWRGYWlsKFN1YnJlc291cmNlTG8KICAgICBSZXF1ZXN0KiByZXF1ZXN0ID0gaS0+ c2Vjb25kOwogICAgIG1fcmVxdWVzdHNMb2FkaW5nLnJlbW92ZShpKTsKICAgICBEb2NMb2FkZXIq IGRvY0xvYWRlciA9IHJlcXVlc3QtPmRvY0xvYWRlcigpOworICAgIC8vIFByZXZlbnQgdGhlIGRv Y3VtZW50IGZyb20gYmVpbmcgZGVzdHJveWVkIGJlZm9yZSB3ZSBhcmUgZG9uZSB3aXRoCisgICAg Ly8gdGhlIGRvY0xvYWRlciB0aGF0IGl0IHdpbGwgZGVsZXRlIHdoZW4gdGhlIGRvY3VtZW50IGdl dHMgZGVsZXRlZC4KKyAgICBEb2NQdHI8RG9jdW1lbnQ+IHByb3RlY3Rvcihkb2NMb2FkZXItPmRv YygpKTsKICAgICBpZiAoIXJlcXVlc3QtPmlzTXVsdGlwYXJ0KCkpCiAgICAgICAgIGRvY0xvYWRl ci0+ZGVjcmVtZW50UmVxdWVzdENvdW50KCk7CiAK