38543 2010-05-04 13:46:08 -0700 [Chromium] Crash in WebPageSerializerImpl::serialize() on downloaded iframe 2010-05-12 12:15:26 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 japhet japhet abarth dglazkov eric webkit.review.bot oldest_to_newest 220744 0 japhet 2010-05-04 13:46:08 -0700 Original report at http://crbug.com/42212 If an iframe is set to a url that gets downloaded, the frame loader never commits to the load and never sets m_URL (since it never actually performed a navigation). If we try to save the page, we get to WebPageSerializerImpl::serialize() and try to do operations on each of the urls in turn, reach the invalid url of the downloaded iframe, and crash. There are two ways to fix this: 1. Check the url in WebPageSerializerImpl before using it 2. Ensure FrameLoader::m_url is always valid. I'm inclined to go with (1), since this crash appears to be occurring in Chromium only. 220751 1 55044 japhet 2010-05-04 13:54:54 -0700 Created attachment 55044 patch 220760 2 55044 abarth 2010-05-04 14:04:06 -0700 Comment on attachment 55044 patch Please add a test. I think we have the ability to unit test the Chromium WebKit API. 220838 3 japhet 2010-05-04 15:44:31 -0700 A test that covers this functionality would need to bring up a full WebFrame and be able to exercise it. I don't believe that is possible with the testing tools currently available for the Chromium API in webkit.org (I think it requires the tools chromium has in test_shell_tests). If you would like me to write a unit test, I'm happy to do so, but I think it would need to be in src.chromium.org for now. 221128 4 55044 japhet 2010-05-05 08:37:18 -0700 Comment on attachment 55044 patch Resetting r? for the reason outlined in my previous post (a test that covers this functionality would need to live in the chromium repo at the moment). 221141 5 japhet 2010-05-05 09:23:06 -0700 (In reply to comment #4) > (From update of attachment 55044 [details]) > Resetting r? for the reason outlined in my previous post (a test that covers > this functionality would need to live in the chromium repo at the moment). FYI, the test for this patch is up for review at http://codereview.chromium.org/1917007 221483 6 55044 eric 2010-05-05 21:56:23 -0700 Comment on attachment 55044 patch Needs tests or explanation of why testing is impossible. 221692 7 dglazkov 2010-05-06 08:17:29 -0700 (In reply to comment #6) > (From update of attachment 55044 [details]) > Needs tests or explanation of why testing is impossible. In the changelog, right? Because I think Nate provided a pretty good explanation in the bug. 222428 8 eric 2010-05-07 09:35:18 -0700 Yes, in the ChangeLog. I don't (and I guess Adam doesn't either) read bugs during most reviews (except to validate that no one else has objected). 222434 9 55388 japhet 2010-05-07 09:41:03 -0700 Created attachment 55388 patch + updated changelog 224116 10 55388 abarth 2010-05-11 13:05:55 -0700 Comment on attachment 55388 patch + updated changelog ok 224154 11 webkit.review.bot 2010-05-11 14:24:02 -0700 http://trac.webkit.org/changeset/59169 might have broken Chromium Win Release The following changes are on the blame list: http://trac.webkit.org/changeset/59168 http://trac.webkit.org/changeset/59169 http://trac.webkit.org/changeset/59170 http://trac.webkit.org/changeset/59165 http://trac.webkit.org/changeset/59166 http://trac.webkit.org/changeset/59167 224790 12 japhet 2010-05-12 12:15:26 -0700 http://trac.webkit.org/changeset/59169 55044 2010-05-04 13:54:54 -0700 2010-05-07 09:41:03 -0700 patch patch.txt text/plain 1454 japhet SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNTg3NzIpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTAtMDUtMDQgIE5hdGUg Q2hhcGluICA8amFwaGV0QGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBDcmFzaCBmaXggaW4gV2ViUGFnZVNlcmlhbGl6ZXJJbXBs OjpzZXJpYWxpemUoKS4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9Mzg1NDMKKworICAgICAgICAqIHNyYy9XZWJQYWdlU2VyaWFsaXplckltcGwuY3Bw OgorICAgICAgICAoV2ViS2l0OjpXZWJQYWdlU2VyaWFsaXplckltcGw6OnNlcmlhbGl6ZSk6IENo ZWNrIGVhY2ggZnJhbWUncyB1cmwgYmVmb3JlIHVzaW5nIGl0LAorICAgICAgICAgICAgc2luY2Ug dGhleSBhcmUgbm90IGd1YXJhbnRlZWQgdG8gYmUgdmFsaWQgKGUuZy4sIGlmIHRoZSBmcmFtZSB3 YXMgdHJlYXRlZCBhcyBhIGRvd25sb2FkKS4KKwogMjAxMC0wNC0yOSAgSm9obiBHcmVnZyAgPGpv aG5ueWdAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBEbWl0cnkgVGl0b3YuCklu ZGV4OiBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlBhZ2VTZXJpYWxpemVySW1wbC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJQYWdlU2VyaWFsaXplckltcGwuY3BwCShy ZXZpc2lvbiA1ODc3MSkKKysrIFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViUGFnZVNlcmlhbGl6ZXJJ bXBsLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTEwLDcgKzUxMCw3IEBACiAgICAgICAgIGNvbnN0 IEtVUkwmIGN1cnJlbnRGcmFtZVVSTCA9IGN1cnJlbnRGcmFtZS0+ZnJhbWUoKS0+bG9hZGVyKCkt PnVybCgpOwogCiAgICAgICAgIC8vIENoZWNrIHdoZXRoZXIgd2UgaGF2ZSBkb25lIHRoaXMgZG9j dW1lbnQuCi0gICAgICAgIGlmIChtX2xvY2FsTGlua3MuY29udGFpbnMoY3VycmVudEZyYW1lVVJM LnN0cmluZygpKSkgeworICAgICAgICBpZiAoY3VycmVudEZyYW1lVVJMLmlzVmFsaWQoKSAmJiBt X2xvY2FsTGlua3MuY29udGFpbnMoY3VycmVudEZyYW1lVVJMLnN0cmluZygpKSkgewogICAgICAg ICAgICAgLy8gQSBuZXcgZG9jdW1lbnQsIHdlIHdpbGwgc2VyaWFsaXplIGl0LgogICAgICAgICAg ICAgZGlkU2VyaWFsaXphdGlvbiA9IHRydWU7CiAgICAgICAgICAgICAvLyBHZXQgdGFyZ2V0IGVu Y29kaW5nIGZvciBjdXJyZW50IGRvY3VtZW50Lgo= 55388 2010-05-07 09:41:03 -0700 2010-05-11 13:05:54 -0700 patch + updated changelog patch.txt text/plain 1648 japhet SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNTg3NzIpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTAtMDUtMDcgIE5hdGUg Q2hhcGluICA8amFwaGV0QGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBDcmFzaCBmaXggaW4gV2ViUGFnZVNlcmlhbGl6ZXJJbXBs OjpzZXJpYWxpemUoKS4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9Mzg1NDMKKworICAgICAgICBUaGUgcmVsZXZhbnQgdGVzdCBpcyBhIHRlc3Rfc2hl bGxfdGVzdCBpbiBzcmMuY2hyb21pdW0ub3JnLCBiZWNhdXNlIG5laXRoZXIKKyAgICAgICAgRFJU IG5vciB0aGUgQ2hyb21pdW0gd2Via2l0IHVuaXQgdGVzdHMgY2FuIGN1cnJlbnRseSBjb3ZlciB0 aGUgc2VyaWFsaXplcgorICAgICAgICBmdW5jdGlvbmFsaXR5LgorCisgICAgICAgICogc3JjL1dl YlBhZ2VTZXJpYWxpemVySW1wbC5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlBhZ2VTZXJpYWxp emVySW1wbDo6c2VyaWFsaXplKTogQ2hlY2sgZWFjaCBmcmFtZSdzIHVybCBiZWZvcmUgdXNpbmcg aXQsCisgICAgICAgICAgICBzaW5jZSB0aGV5IGFyZSBub3QgZ3VhcmFudGVlZCB0byBiZSB2YWxp ZCAoZS5nLiwgaWYgdGhlIGZyYW1lIHdhcyB0cmVhdGVkIGFzIGEgZG93bmxvYWQpLgorCiAyMDEw LTA0LTI5ICBKb2huIEdyZWdnICA8am9obm55Z0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmll d2VkIGJ5IERtaXRyeSBUaXRvdi4KSW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViUGFnZVNl cmlhbGl6ZXJJbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlBh Z2VTZXJpYWxpemVySW1wbC5jcHAJKHJldmlzaW9uIDU4NzcxKQorKysgV2ViS2l0L2Nocm9taXVt L3NyYy9XZWJQYWdlU2VyaWFsaXplckltcGwuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01MTAsNyAr NTEwLDcgQEAKICAgICAgICAgY29uc3QgS1VSTCYgY3VycmVudEZyYW1lVVJMID0gY3VycmVudEZy YW1lLT5mcmFtZSgpLT5sb2FkZXIoKS0+dXJsKCk7CiAKICAgICAgICAgLy8gQ2hlY2sgd2hldGhl ciB3ZSBoYXZlIGRvbmUgdGhpcyBkb2N1bWVudC4KLSAgICAgICAgaWYgKG1fbG9jYWxMaW5rcy5j b250YWlucyhjdXJyZW50RnJhbWVVUkwuc3RyaW5nKCkpKSB7CisgICAgICAgIGlmIChjdXJyZW50 RnJhbWVVUkwuaXNWYWxpZCgpICYmIG1fbG9jYWxMaW5rcy5jb250YWlucyhjdXJyZW50RnJhbWVV Ukwuc3RyaW5nKCkpKSB7CiAgICAgICAgICAgICAvLyBBIG5ldyBkb2N1bWVudCwgd2Ugd2lsbCBz ZXJpYWxpemUgaXQuCiAgICAgICAgICAgICBkaWRTZXJpYWxpemF0aW9uID0gdHJ1ZTsKICAgICAg ICAgICAgIC8vIEdldCB0YXJnZXQgZW5jb2RpbmcgZm9yIGN1cnJlbnQgZG9jdW1lbnQuCg==