24398 2009-03-05 12:26:02 -0800 BackForwardList doesn't initialize m_client, which causes a crash 2010-02-24 17:38:06 -0800 1 1 1 Unclassified WebKit History 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 maruel maruel hclam mrowe oldest_to_newest 112423 0 maruel 2009-03-05 12:26:02 -0800 In the chromium port, when loading test_shell with a svg file, reload the "page" will crash test_shell. 112424 1 28315 maruel 2009-03-05 12:26:40 -0800 Created attachment 28315 proposed patch 112551 2 mrowe 2009-03-06 00:10:12 -0800 The header for BackForwardList contains the following: #if PLATFORM(CHROMIUM) // Must be called before any other methods. void setClient(BackForwardListClient* client) { m_client = client; } #endif This implies that your code is using this class incorrectly. Of course, it may be that the existing design of the class is not ideal and it should be changed, but if that is the case the comment is no longer correct. 112878 3 maruel 2009-03-09 12:48:25 -0700 What is happening in that particular case is that SVGImage::dataChanged() creates a new dummy Page() which doesn't have any back-forward logic. I think null-initializing is fine in this case since ~Page() always calls m_backForwardList->close(). I can update the comment in BackForwardList.h for BackForwardListClient::setClient() to // Must be called before any other methods except close(). 112883 4 28315 fishd 2009-03-09 13:03:44 -0700 Comment on attachment 28315 proposed patch >Index: WebCore/ChangeLog ... >+ Reviewed by NOBODY (OOPS!). >+ >+ Fix a crash when loading a non html page (like a svg file) in test_shell >+ and reloading the page. >+ >+ * history/BackForwardListChromium.cpp: please add a bug link to the ChangeLog. >Index: WebCore/history/BackForwardListChromium.cpp ... >+ if (m_client) >+ m_client->close(); we should probably null check each usage and probably add some ASSERTs. wdyt? 112886 5 28418 maruel 2009-03-09 13:25:49 -0700 Created attachment 28418 Fixed ChangeLog 112888 6 maruel 2009-03-09 13:27:45 -0700 I don't think it is a good idea to NULL check each functions and add ASSERTs. Having m_client null-initialized will help there and this was a very particular case. I looked at directly extracting m_client from m_page and it is very deep down multiple objects hierarchy. 113138 7 zecke 2009-03-11 02:00:24 -0700 Please don't use NULL in C++ code. 113430 8 28418 fishd 2009-03-12 11:33:41 -0700 Comment on attachment 28418 Fixed ChangeLog >Index: WebCore/history/BackForwardListChromium.cpp ... >+ , m_client(NULL) As mentioned, this should be 0 instead of NULL. Please post a revised patch, and then LGTM. 113629 9 28587 maruel 2009-03-13 11:22:22 -0700 Created attachment 28587 new patch file Replace NULL with 0. 114282 10 fishd 2009-03-18 23:18:37 -0700 Landed as http://trac.webkit.org/changeset/41824 193578 11 hclam 2010-02-24 17:38:06 -0800 *** Bug 24636 has been marked as a duplicate of this bug. *** 28315 2009-03-05 12:26:40 -0800 2009-03-09 13:25:49 -0700 proposed patch backforward.diff text/plain 1317 maruel SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MTQ1NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDktMDMtMDUgIE1hcmMtQW50b2luZSBSdWVsICA8bWFydWVsQGNo cm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBGaXggYSBjcmFzaCB3aGVuIGxvYWRpbmcgYSBub24gaHRtbCBwYWdlIChsaWtlIGEgc3Zn IGZpbGUpIGluIHRlc3Rfc2hlbGwKKyAgICAgICAgYW5kIHJlbG9hZGluZyB0aGUgcGFnZS4KKwor ICAgICAgICAqIGhpc3RvcnkvQmFja0ZvcndhcmRMaXN0Q2hyb21pdW0uY3BwOgorICAgICAgICAo V2ViQ29yZTo6QmFja0ZvcndhcmRMaXN0OjpCYWNrRm9yd2FyZExpc3QpOgorICAgICAgICAoV2Vi Q29yZTo6QmFja0ZvcndhcmRMaXN0OjpjbG9zZSk6CisKIDIwMDktMDMtMDUgIFlvbmcgTGkgIDx5 b25nLmxpQHRvcmNobW9iaWxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBGcmFz ZXIuCkluZGV4OiBXZWJDb3JlL2hpc3RvcnkvQmFja0ZvcndhcmRMaXN0Q2hyb21pdW0uY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFdlYkNvcmUvaGlzdG9yeS9CYWNrRm9yd2FyZExpc3RDaHJvbWl1bS5jcHAJ KHJldmlzaW9uIDQxNDU1KQorKysgV2ViQ29yZS9oaXN0b3J5L0JhY2tGb3J3YXJkTGlzdENocm9t aXVtLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzgsNiArMzgsNyBAQCBzdGF0aWMgY29uc3QgdW5z aWduZWQgTm9DdXJyZW50SXRlbUluZGV4CiAKIEJhY2tGb3J3YXJkTGlzdDo6QmFja0ZvcndhcmRM aXN0KFBhZ2UqIHBhZ2UpCiAgICAgOiBtX3BhZ2UocGFnZSkKKyAgICAsIG1fY2xpZW50KE5VTEwp CiAgICAgLCBtX2NhcGFjaXR5KERlZmF1bHRDYXBhY2l0eSkKICAgICAsIG1fY2xvc2VkKHRydWUp CiAgICAgLCBtX2VuYWJsZWQodHJ1ZSkKQEAgLTEyOCw3ICsxMjksOCBAQCBIaXN0b3J5SXRlbVZl Y3RvciYgQmFja0ZvcndhcmRMaXN0OjplbnRyCiAKIHZvaWQgQmFja0ZvcndhcmRMaXN0OjpjbG9z ZSgpCiB7Ci0gICAgbV9jbGllbnQtPmNsb3NlKCk7CisgICAgaWYgKG1fY2xpZW50KQorICAgICAg ICBtX2NsaWVudC0+Y2xvc2UoKTsKICAgICBtX3BhZ2UgPSAwOwogICAgIG1fY2xvc2VkID0gdHJ1 ZTsKIH0K 28418 2009-03-09 13:25:49 -0700 2009-03-12 11:33:41 -0700 Fixed ChangeLog b24398.diff text/plain 1357 maruel SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MTQ3OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDMtMDUgIE1hcmMtQW50b2luZSBSdWVsICA8bWFydWVsQGNo cm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjQzOTgKKyAgICAg ICAgRml4IGEgY3Jhc2ggd2hlbiBsb2FkaW5nIGEgc3ZnIGZpbGUgaW4gdGVzdF9zaGVsbAorICAg ICAgICBhbmQgcmVsb2FkaW5nIHRoZSBwYWdlLgorCisgICAgICAgICogaGlzdG9yeS9CYWNrRm9y d2FyZExpc3RDaHJvbWl1bS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpCYWNrRm9yd2FyZExpc3Q6 OkJhY2tGb3J3YXJkTGlzdCk6CisgICAgICAgIChXZWJDb3JlOjpCYWNrRm9yd2FyZExpc3Q6OmNs b3NlKToKKwogMjAwOS0wMy0wNiAgSGlyb25vcmkgQm9ubyAgPGhib25vQGNocm9taXVtLm9yZz4K IAogICAgICAgICBSZXZpZXdlZCBieSBBbGV4ZXkgUHJvc2t1cnlha292LgpJbmRleDogV2ViQ29y ZS9oaXN0b3J5L0JhY2tGb3J3YXJkTGlzdENocm9taXVtLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJD b3JlL2hpc3RvcnkvQmFja0ZvcndhcmRMaXN0Q2hyb21pdW0uY3BwCShyZXZpc2lvbiA0MTQ3OSkK KysrIFdlYkNvcmUvaGlzdG9yeS9CYWNrRm9yd2FyZExpc3RDaHJvbWl1bS5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTM4LDYgKzM4LDcgQEAgc3RhdGljIGNvbnN0IHVuc2lnbmVkIE5vQ3VycmVudEl0 ZW1JbmRleAogCiBCYWNrRm9yd2FyZExpc3Q6OkJhY2tGb3J3YXJkTGlzdChQYWdlKiBwYWdlKQog ICAgIDogbV9wYWdlKHBhZ2UpCisgICAgLCBtX2NsaWVudChOVUxMKQogICAgICwgbV9jYXBhY2l0 eShEZWZhdWx0Q2FwYWNpdHkpCiAgICAgLCBtX2Nsb3NlZCh0cnVlKQogICAgICwgbV9lbmFibGVk KHRydWUpCkBAIC0xMjgsNyArMTI5LDggQEAgSGlzdG9yeUl0ZW1WZWN0b3ImIEJhY2tGb3J3YXJk TGlzdDo6ZW50cgogCiB2b2lkIEJhY2tGb3J3YXJkTGlzdDo6Y2xvc2UoKQogewotICAgIG1fY2xp ZW50LT5jbG9zZSgpOworICAgIGlmIChtX2NsaWVudCkKKyAgICAgICAgbV9jbGllbnQtPmNsb3Nl KCk7CiAgICAgbV9wYWdlID0gMDsKICAgICBtX2Nsb3NlZCA9IHRydWU7CiB9Cg== 28587 2009-03-13 11:22:22 -0700 2009-03-18 23:09:01 -0700 new patch file b24398_2.diff text/plain 1354 maruel SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MTQ3OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDMtMDUgIE1hcmMtQW50b2luZSBSdWVsICA8bWFydWVsQGNo cm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjQzOTgKKyAgICAg ICAgRml4IGEgY3Jhc2ggd2hlbiBsb2FkaW5nIGEgc3ZnIGZpbGUgaW4gdGVzdF9zaGVsbAorICAg ICAgICBhbmQgcmVsb2FkaW5nIHRoZSBwYWdlLgorCisgICAgICAgICogaGlzdG9yeS9CYWNrRm9y d2FyZExpc3RDaHJvbWl1bS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpCYWNrRm9yd2FyZExpc3Q6 OkJhY2tGb3J3YXJkTGlzdCk6CisgICAgICAgIChXZWJDb3JlOjpCYWNrRm9yd2FyZExpc3Q6OmNs b3NlKToKKwogMjAwOS0wMy0wNiAgSGlyb25vcmkgQm9ubyAgPGhib25vQGNocm9taXVtLm9yZz4K IAogICAgICAgICBSZXZpZXdlZCBieSBBbGV4ZXkgUHJvc2t1cnlha292LgpJbmRleDogV2ViQ29y ZS9oaXN0b3J5L0JhY2tGb3J3YXJkTGlzdENocm9taXVtLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJD b3JlL2hpc3RvcnkvQmFja0ZvcndhcmRMaXN0Q2hyb21pdW0uY3BwCShyZXZpc2lvbiA0MTQ3OSkK KysrIFdlYkNvcmUvaGlzdG9yeS9CYWNrRm9yd2FyZExpc3RDaHJvbWl1bS5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTM4LDYgKzM4LDcgQEAgc3RhdGljIGNvbnN0IHVuc2lnbmVkIE5vQ3VycmVudEl0 ZW1JbmRleAogCiBCYWNrRm9yd2FyZExpc3Q6OkJhY2tGb3J3YXJkTGlzdChQYWdlKiBwYWdlKQog ICAgIDogbV9wYWdlKHBhZ2UpCisgICAgLCBtX2NsaWVudCgwKQogICAgICwgbV9jYXBhY2l0eShE ZWZhdWx0Q2FwYWNpdHkpCiAgICAgLCBtX2Nsb3NlZCh0cnVlKQogICAgICwgbV9lbmFibGVkKHRy dWUpCkBAIC0xMjgsNyArMTI5LDggQEAgSGlzdG9yeUl0ZW1WZWN0b3ImIEJhY2tGb3J3YXJkTGlz dDo6ZW50cgogCiB2b2lkIEJhY2tGb3J3YXJkTGlzdDo6Y2xvc2UoKQogewotICAgIG1fY2xpZW50 LT5jbG9zZSgpOworICAgIGlmIChtX2NsaWVudCkKKyAgICAgICAgbV9jbGllbnQtPmNsb3NlKCk7 CiAgICAgbV9wYWdlID0gMDsKICAgICBtX2Nsb3NlZCA9IHRydWU7CiB9Cg==