52512 2011-01-15 11:27:42 -0800 REGRESSION (r73818): range.cloneContents() ignores end offset 2011-02-21 02:20:50 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED HasReduction, InRadar, Regression P1 Normal --- 50854 52524 1 ddkilzer ddkilzer abarth adele aroben darin inferno jberlin mitz rniwa oldest_to_newest 334909 0 79068 ddkilzer 2011-01-15 11:27:42 -0800 Created attachment 79068 Patch Reviewed by NOBODY (OOPS!). WebCore: The fix for Bug 50710 in r73799 introduced an off-by-one error when copying nodes to a local NodeVector for processing. A fix was attempted for Bug 50854 in r73818, but instead of stopping at the end offset, it iterates through all the sibling nodes because the loop variable (i) is never incremented. To clean this up, revert back to the code in r73799 and fix the off-by-one error. Test: fast/dom/Range/range-clone-contents.html * dom/Range.cpp: (WebCore::Range::processContents): Fix the loop that copies nodes to a local NodeVector by restoring the code from r73799 and fixing the off-by-one error. LayoutTests: * fast/dom/Range/range-clone-contents-expected.txt: Added. * fast/dom/Range/range-clone-contents.html: Added. --- 5 files changed, 124 insertions(+), 6 deletions(-) 334911 1 ddkilzer 2011-01-15 11:35:04 -0800 Note to reviewers: Please verify the new algorithm matches the behavior of the original code. I've gone through the new algorithm, but this code has had bugs introduced through two rewrites and two reviews, so please study it carefully before giving an r+. This is the pre-r73799 code in processContents() in WebCore/dom/Range.cpp: Node* n = m_end.container()->firstChild(); if (n && m_end.offset()) { for (int i = 0; i + 1 < m_end.offset(); i++) { // skip to end.offset() Node* next = n->nextSibling(); if (!next) break; n = next; } Node* prev; for (; n; n = prev) { prev = n->previousSibling(); [...] } [...] } The fix is this code: Node* n = m_end.container()->firstChild(); if (n && m_end.offset()) { NodeVector nodes; for (int i = 0; i < m_end.offset() && n; i++, n = n->nextSibling()) nodes.append(n); for (int i = nodes.size() - 1; i >= 0; i--) { n = nodes[i].get(); [...] } [...] } 334914 2 ddkilzer 2011-01-15 11:42:50 -0800 <rdar://problem/8789045> 334924 3 79068 adele 2011-01-15 12:40:32 -0800 Comment on attachment 79068 Patch I'm not sure I understand the intermediate change before this very well, but this code looks right. You're still going to always append at least the end container's first child, since m_end.offset() won't be 0 in the inner loop. The new test looks good too, and Dave says this also passes the other editing tests, including the one that was failing for one of the earlier patches. r=me! 334926 4 ddkilzer 2011-01-15 12:45:09 -0800 Committed r75882: <http://trac.webkit.org/changeset/75882> 334944 5 jberlin 2011-01-15 14:24:01 -0800 (In reply to comment #4) > Committed r75882: <http://trac.webkit.org/changeset/75882> fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html 334974 6 ddkilzer 2011-01-15 17:19:07 -0800 (In reply to comment #5) > (In reply to comment #4) > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html This is the exact erroneous behavior that the test is trying to fix (where the <img> tag is left in the range instead of being removed). It's as if Range.cpp wasn't compiled on the Windows 7 bot. 334976 7 ddkilzer 2011-01-15 17:41:20 -0800 (In reply to comment #5) > (In reply to comment #4) > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html Same issue on the Windows XP box: http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r75882%20(24216)/fast/dom/Range/range-clone-contents-pretty-diff.html Oddly enough, I don't see any other bots failing on this test. 334983 8 ddkilzer 2011-01-15 18:16:31 -0800 (In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #4) > > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > [...] > Same issue on the Windows XP box: > [...] > Oddly enough, I don't see any other bots failing on this test. Filed Bug 52524 to cover this. Added the test to the Windows skip list: Committed r75889. <http://trac.webkit.org/changeset/75889> 335583 9 ddkilzer 2011-01-17 16:51:08 -0800 (In reply to comment #8) > Filed Bug 52524 to cover this. Added the test to the Windows skip list: > > Committed r75889. <http://trac.webkit.org/changeset/75889> Turns out the buildbots just needed to do a clean build. Removed the test from the skip list: Committed r75970: <http://trac.webkit.org/changeset/75970> 354480 10 rniwa 2011-02-21 02:20:50 -0800 *** Bug 54861 has been marked as a duplicate of this bug. *** 79068 2011-01-15 11:27:42 -0800 2011-01-15 12:40:32 -0800 Patch 20110115112741.patch text/plain 5735 ddkilzer ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCAzYzhjZThmYTk5OWJhMjY5MDM5NTlmMWFiYjA3NTExZmI5MDQ4ZGUwLi5iYzJmODE0 NWE4YmI1NDQzYmRlZWVmZTYxMmI3NjQ2NjU4Zjc0Y2Y3IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0 cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAK KzIwMTEtMDEtMTUgIERhdmlkIEtpbHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAg ICBSRUdSRVNTSU9OKHI3MzgxOCk6IHJhbmdlLmNsb25lQ29udGVudHMoKSBpZ25vcmVzIGVuZCBv ZmZzZXQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IGZhc3QvZG9tL1JhbmdlL3JhbmdlLWNsb25lLWNvbnRlbnRzLWV4cGVjdGVkLnR4dDogQWRkZWQu CisgICAgICAgICogZmFzdC9kb20vUmFuZ2UvcmFuZ2UtY2xvbmUtY29udGVudHMuaHRtbDogQWRk ZWQuCisKIDIwMTEtMDEtMTUgIFJvYmVydCBIb2dhbiAgPHJvYmVydEB3ZWJraXQub3JnPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IEtlbm5ldGggUm9oZGUgQ2hyaXN0aWFuc2VuLgpkaWZmIC0tZ2l0 IGEvTGF5b3V0VGVzdHMvZmFzdC9kb20vUmFuZ2UvcmFuZ2UtY2xvbmUtY29udGVudHMtZXhwZWN0 ZWQudHh0IGIvTGF5b3V0VGVzdHMvZmFzdC9kb20vUmFuZ2UvcmFuZ2UtY2xvbmUtY29udGVudHMt ZXhwZWN0ZWQudHh0Cm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLmYwZDUwZGYwODRhMmY5MDM5MTUzM2Y5YWQzODUxYTIx ZWZiZDRlOTMKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0L2RvbS9SYW5nZS9y YW5nZS1jbG9uZS1jb250ZW50cy1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwzNiBAQAordGVzdCBj b250ZW50czoKK3wgPGRpdj4KK3wgICA8YT4KK3wgICAgIGhyZWY9IiMiCit8ICAgICAibGluayIK K3wgPGRpdj4KK3wgPGRpdj4KK3wgICA8YnI+Cit8ICAgPGJyPgorfCAgIDxpbWc+Cit8ICAgICBp ZD0iaW1nLXRhZyIKK3wgICAgIHNyYz0iIgorfCA8ZGl2PgorfCAgIDxicj4KK3wgICA8YnI+Cit8 ICAgInRleHQiCisKK1BBU1M6IENsb25pbmcgcmlnaHQgY29tcGxleCBzdWJ0cmVlIHlpZWxkczoK K3wgPGRpdj4KK3wgICA8YT4KK3wgICAgIGhyZWY9IiMiCit8ICAgICAibGluayIKK3wgPGRpdj4K K3wgPGRpdj4KK3wgICA8YnI+Cit8ICAgPGJyPgorCitQQVNTOiBDbG9uaW5nIGxlZnQgY29tcGxl eCBzdWJ0cmVlIHlpZWxkczoKK3wgPGRpdj4KK3wgPGRpdj4KK3wgICA8YnI+Cit8ICAgPGJyPgor fCAgICJ0ZXh0IgorCitET05FCisKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3QvZG9tL1Jh bmdlL3JhbmdlLWNsb25lLWNvbnRlbnRzLmh0bWwgYi9MYXlvdXRUZXN0cy9mYXN0L2RvbS9SYW5n ZS9yYW5nZS1jbG9uZS1jb250ZW50cy5odG1sCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjJkZjdkZmQ0ZjU4ODM4YTg3 NzNkOTllODcxM2MzYWY3ZmQ2N2Y0MjkKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9m YXN0L2RvbS9SYW5nZS9yYW5nZS1jbG9uZS1jb250ZW50cy5odG1sCkBAIC0wLDAgKzEsNTcgQEAK KzwhRE9DVFlQRSBodG1sPgorPGh0bWw+Cis8aGVhZD4KKzxzY3JpcHQgc3JjPSIuLi8uLi8uLi9y ZXNvdXJjZXMvZHVtcC1hcy1tYXJrdXAuanMiPjwvc2NyaXB0PgorPC9oZWFkPgorPGJvZHk+Cis8 ZGl2IGlkPSJ0ZXN0Ij48ZGl2PjxhIGhyZWY9IiMiPmxpbms8L2E+PC9kaXY+PGRpdj48L2Rpdj48 ZGl2Pjxicj48YnI+PGltZyBzcmM9IiIgaWQ9ImltZy10YWciPjwvZGl2PjxkaXY+PGJyPjxicj50 ZXh0PC9kaXY+PC9kaXY+Cis8ZGl2IGlkPSJleHBlY3RhdGlvbnMiPgorICAgIDxkaXYgaWQ9InRl c3RfcmlnaHRfY29tcGxleF9zdWJ0cmVlIj48ZGl2PjxhIGhyZWY9IiMiPmxpbms8L2E+PC9kaXY+ PGRpdj48L2Rpdj48ZGl2Pjxicj48YnI+PC9kaXY+PC9kaXY+CisgICAgPGRpdiBpZD0idGVzdF9s ZWZ0X2NvbXBsZXhfc3VidHJlZSI+PGRpdj48L2Rpdj48ZGl2Pjxicj48YnI+dGV4dDwvZGl2Pjwv ZGl2PgorPC9kaXY+Cis8cHJlIGlkPSJjb25zb2xlIj48L3ByZT4KKzxzY3JpcHQ+CisKK2Z1bmN0 aW9uIGxvZyhtZXNzYWdlKSB7CisgICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoJ2NvbnNvbGUn KS5pbm5lckhUTUwgKz0gbWVzc2FnZSArICdcbic7Cit9CisKK2Z1bmN0aW9uIHRlc3RDbG9uZUNv bnRlbnRzKGRlc2NyaXB0aW9uLCByYW5nZSwgZXhwZWN0ZWRDb250ZW50c0lkKSB7CisgICAgdmFy IGFjdHVhbENvbnRlbnRzID0gcmFuZ2UuY2xvbmVDb250ZW50cygpOworCisgICAgdmFyIGFjdGlv biA9IGRlc2NyaXB0aW9uICsgJyB5aWVsZHM6XG4nICsgTWFya3VwLmdldChhY3R1YWxDb250ZW50 cykucmVwbGFjZSgvPC9nLCAnJmx0OycpOworICAgIHZhciBleHBlY3RlZENvbnRlbnRzID0gZG9j dW1lbnQuZ2V0RWxlbWVudEJ5SWQoZXhwZWN0ZWRDb250ZW50c0lkKTsKKyAgICBpZiAoTWFya3Vw LmdldChhY3R1YWxDb250ZW50cykgPT0gTWFya3VwLmdldChleHBlY3RlZENvbnRlbnRzKSkKKyAg ICAgICAgbG9nKCdQQVNTOiAnICsgYWN0aW9uKTsKKyAgICBlbHNlCisgICAgICAgIGxvZygnRkFJ TDogJyArIGFjdGlvbiArICdcbiBidXQgZXhwZWN0ZWQ6XG4nICsgTWFya3VwLmdldChleHBlY3Rl ZENvbnRlbnRzKS5yZXBsYWNlKC88L2csICcmbHQ7JykpOworCisgICAgbG9nKCcnKTsKK30KKwor TWFya3VwLm5vQXV0b0R1bXAoKTsKKwordmFyIHRlc3QgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJ ZCgndGVzdCcpOworbG9nKCd0ZXN0IGNvbnRlbnRzOlxuJyArIE1hcmt1cC5nZXQodGVzdCkucmVw bGFjZSgvPC9nLCAnJmx0OycpICsgJ1xuJykKKwordmFyIHJhbmdlID0gZG9jdW1lbnQuY3JlYXRl UmFuZ2UoKTsKKworcmFuZ2Uuc2V0U3RhcnRCZWZvcmUodGVzdC5maXJzdENoaWxkKTsKK3ZhciBp bWdUYWcgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgnaW1nLXRhZycpCityYW5nZS5zZXRFbmRC ZWZvcmUoaW1nVGFnKTsKKwordGVzdENsb25lQ29udGVudHMoJ0Nsb25pbmcgcmlnaHQgY29tcGxl eCBzdWJ0cmVlJywgcmFuZ2UsICd0ZXN0X3JpZ2h0X2NvbXBsZXhfc3VidHJlZScpOworCityYW5n ZS5zZXRTdGFydEFmdGVyKGltZ1RhZyk7CityYW5nZS5zZXRFbmRBZnRlcih0ZXN0Lmxhc3RDaGls ZCk7CisKK3Rlc3RDbG9uZUNvbnRlbnRzKCdDbG9uaW5nIGxlZnQgY29tcGxleCBzdWJ0cmVlJywg cmFuZ2UsICd0ZXN0X2xlZnRfY29tcGxleF9zdWJ0cmVlJyk7CisKK3Rlc3Quc3R5bGUuZGlzcGxh eSA9ICdub25lJzsKK2RvY3VtZW50LmdldEVsZW1lbnRCeUlkKCdleHBlY3RhdGlvbnMnKS5zdHls ZS5kaXNwbGF5ID0gJ25vbmUnOworCitsb2coJ0RPTkUnKTsKKworPC9zY3JpcHQ+Cis8L2JvZHk+ Cis8L2h0bWw+CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2Uv V2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTgxZWU3YzQwMGQ3YjNiYjkwMDM2OGEzZTM5ZDY5ZTU5 MTJkODM5ZS4uZjU3ZTg5MDQ4NTUxOTYwYWY3MGNlNGNmYTgxMDI0ZWFhZjBhNTAyMSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDExLTAxLTE1ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHpl ckBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTihyNzM4MTgpOiByYW5nZS5jbG9uZUNv bnRlbnRzKCkgaWdub3JlcyBlbmQgb2Zmc2V0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVGhlIGZpeCBmb3IgQnVnIDUwNzEwIGluIHI3Mzc5OSBpbnRy b2R1Y2VkIGFuIG9mZi1ieS1vbmUgZXJyb3IKKyAgICAgICAgd2hlbiBjb3B5aW5nIG5vZGVzIHRv IGEgbG9jYWwgTm9kZVZlY3RvciBmb3IgcHJvY2Vzc2luZy4gIEEgZml4CisgICAgICAgIHdhcyBh dHRlbXB0ZWQgZm9yIEJ1ZyA1MDg1NCBpbiByNzM4MTgsIGJ1dCBpbnN0ZWFkIG9mIHN0b3BwaW5n CisgICAgICAgIGF0IHRoZSBlbmQgb2Zmc2V0LCBpdCBpdGVyYXRlcyB0aHJvdWdoIGFsbCB0aGUg c2libGluZyBub2RlcworICAgICAgICBiZWNhdXNlIHRoZSBsb29wIHZhcmlhYmxlIChpKSBpcyBu ZXZlciBpbmNyZW1lbnRlZC4gIFRvIGNsZWFuCisgICAgICAgIHRoaXMgdXAsIHJldmVydCBiYWNr IHRvIHRoZSBjb2RlIGluIHI3Mzc5OSBhbmQgZml4IHRoZQorICAgICAgICBvZmYtYnktb25lIGVy cm9yLgorCisgICAgICAgIFRlc3Q6IGZhc3QvZG9tL1JhbmdlL3JhbmdlLWNsb25lLWNvbnRlbnRz Lmh0bWwKKworICAgICAgICAqIGRvbS9SYW5nZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSYW5n ZTo6cHJvY2Vzc0NvbnRlbnRzKTogRml4IHRoZSBsb29wIHRoYXQgY29waWVzCisgICAgICAgIG5v ZGVzIHRvIGEgbG9jYWwgTm9kZVZlY3RvciBieSByZXN0b3JpbmcgdGhlIGNvZGUgZnJvbSByNzM3 OTkKKyAgICAgICAgYW5kIGZpeGluZyB0aGUgb2ZmLWJ5LW9uZSBlcnJvci4KKwogMjAxMS0wMS0x NCAgU2FtIE1hZ251c29uICA8c21hZ251c29AZ21haWwuY29tPgogCiAgICAgICAgIFJldmlld2Vk IGJ5IEtlbm5ldGggUm9oZGUgQ2hyaXN0aWFuc2VuLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvZG9tL1JhbmdlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9SYW5nZS5jcHAKaW5kZXggMWYx NWYyNmVmZDVjYTgzNGIyNWZiYWI4YjMzZTk4MTQwMDczNjZlMC4uYTQxZmM0OWExMTY0MzcyNGY5 YjE3ZGRjZGQ2M2E3OTI3OWJkMGY5ZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL1Jh bmdlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20vUmFuZ2UuY3BwCkBAIC03OTQsMTMgKzc5 NCw4IEBAIFBhc3NSZWZQdHI8RG9jdW1lbnRGcmFnbWVudD4gUmFuZ2U6OnByb2Nlc3NDb250ZW50 cyhBY3Rpb25UeXBlIGFjdGlvbiwgRXhjZXB0aW9uCiAgICAgICAgICAgICBOb2RlKiBuID0gbV9l bmQuY29udGFpbmVyKCktPmZpcnN0Q2hpbGQoKTsKICAgICAgICAgICAgIGlmIChuICYmIG1fZW5k Lm9mZnNldCgpKSB7CiAgICAgICAgICAgICAgICAgTm9kZVZlY3RvciBub2RlczsKLSAgICAgICAg ICAgICAgICBpbnQgaSA9IDA7Ci0gICAgICAgICAgICAgICAgZG8geworICAgICAgICAgICAgICAg IGZvciAoaW50IGkgPSAwOyBpIDwgbV9lbmQub2Zmc2V0KCkgJiYgbjsgaSsrLCBuID0gbi0+bmV4 dFNpYmxpbmcoKSkKICAgICAgICAgICAgICAgICAgICAgbm9kZXMuYXBwZW5kKG4pOwotICAgICAg ICAgICAgICAgICAgICBpZiAoIW4tPm5leHRTaWJsaW5nKCkpCi0gICAgICAgICAgICAgICAgICAg ICAgICBicmVhazsKLSAgICAgICAgICAgICAgICAgICAgbiA9IG4tPm5leHRTaWJsaW5nKCk7Ci0g ICAgICAgICAgICAgICAgfSB3aGlsZSAoaSArIDEgPCBtX2VuZC5vZmZzZXQoKSk7CiAgICAgICAg ICAgICAgICAgZm9yIChpbnQgaSA9IG5vZGVzLnNpemUoKSAtIDE7IGkgPj0gMDsgaS0tKSB7CiAg ICAgICAgICAgICAgICAgICAgIG4gPSBub2Rlc1tpXS5nZXQoKTsKICAgICAgICAgICAgICAgICAg ICAgaWYgKGFjdGlvbiA9PSBFWFRSQUNUX0NPTlRFTlRTKQo=