91334 2012-07-14 22:07:09 -0700 REGRESSION(r122660): Cannot iterate over HTMLCollection that contains non-child descendent nodes in some conditions 2012-07-18 14:31:51 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 91306 91335 1 rniwa rniwa andersca ap kling koivisto ojan oldest_to_newest 669925 0 rniwa 2012-07-14 22:07:09 -0700 REGRESSION(r122660): Cannot iterate over HTMLCollection that contains non-child descendent nodes in some conditions 669926 1 152449 rniwa 2012-07-14 22:17:55 -0700 Created attachment 152449 Patch 669927 2 rniwa 2012-07-14 22:19:39 -0700 My attempt to unify HTMLCollection & DynamicNodeList revealed a regression. 669978 3 152449 ojan 2012-07-15 09:32:06 -0700 Comment on attachment 152449 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review Please make the test actually assert that we're iterating in the right order before committing. > Source/WebCore/ChangeLog:10 > + which is visited traverseNextNode immeidatley before reaching the root node. typo: immediately > Source/WebCore/html/HTMLCollection.cpp:254 > + node = node->lastChild(); > + for (Node* current = node; current; current = current->lastChild()) > + node = current; > + return node; nit: i think this would be more readable as a while loop: Node* node = 0; while(Node* next = node->lastChild()) node = next; return node; > LayoutTests/fast/dom/htmlcollection-backwards-subtree-iteration-expected.txt:1 > +Tests that HTMLCollection of a subtree (as supposed to direct children) can be iterated backwards. s/supposed/opposed > LayoutTests/fast/dom/htmlcollection-backwards-subtree-iteration.html:10 > + images[0].class = 'foo'; > + images[i - 1].class = 'foo'; Would be nice if you asserted that you're getting the image elements in the right order. You could put an ID on each one in order to verify it's location. 670037 4 152449 rniwa 2012-07-15 19:00:18 -0700 Comment on attachment 152449 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review >> Source/WebCore/html/HTMLCollection.cpp:254 >> + return node; > > nit: i think this would be more readable as a while loop: > Node* node = 0; > while(Node* next = node->lastChild()) > node = next; > return node; That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. 670043 5 rniwa 2012-07-15 19:26:08 -0700 Committed r122689: <http://trac.webkit.org/changeset/122689> 672741 6 ojan 2012-07-18 14:18:44 -0700 (In reply to comment #4) > (From update of attachment 152449 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review > > >> Source/WebCore/html/HTMLCollection.cpp:254 > >> + return node; > > > > nit: i think this would be more readable as a while loop: > > Node* node = 0; > > while(Node* next = node->lastChild()) > > node = next; > > return node; > > That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. Isn't that what this code does? I don't see where the crash is. 672745 7 rniwa 2012-07-18 14:20:26 -0700 (In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 152449 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review > > > > >> Source/WebCore/html/HTMLCollection.cpp:254 > > >> + return node; > > > > > > nit: i think this would be more readable as a while loop: > > > Node* node = 0; > > > while(Node* next = node->lastChild()) > > > node = next; > > > return node; > > > > That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. > > Isn't that what this code does? I don't see where the crash is. You're initializing node to 0. The first call to node->lastChild() will result in a null pointer access. 672762 8 152449 ojan 2012-07-18 14:31:51 -0700 Comment on attachment 152449 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review >>>>> Source/WebCore/html/HTMLCollection.cpp:254 >>>>> + return node; >>>> >>>> nit: i think this would be more readable as a while loop: >>>> Node* node = 0; >>>> while(Node* next = node->lastChild()) >>>> node = next; >>>> return node; >>> >>> That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. >> >> Isn't that what this code does? I don't see where the crash is. > > You're initializing node to 0. The first call to node->lastChild() will result in a null pointer access. lol. whoops. 152449 2012-07-14 22:17:55 -0700 2012-07-18 14:31:50 -0700 Patch bug-91334-20120714221754.patch text/plain 4997 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyMjY3NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDEyLTA3LTE0ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFJFR1JFU1NJT04ocjEyMjY2MCk6 IENhbm5vdCBpdGVyYXRlIG92ZXIgSFRNTENvbGxlY3Rpb24gdGhhdCBjb250YWlucyBub24tY2hp bGQgZGVzY2VuZGVudCBub2RlcyBpbiBzb21lIGNvbmRpdGlvbnMKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTkxMzM0CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGJ1ZyB3YXMgY2F1c2VkIGJ5IHVzaW5n IGxhc3RDaGlsZCgpIGFzIHRoZSBzdGFydGluZyBub2RlIGZvciB0cmF2ZXJzZVByZXZpb3VzTm9k ZS4gU2luY2UgaXQncyB0aGUgaW52ZXJzZSBvZgorICAgICAgICBOb2RlOjp0cmF2ZXJzZU5leHRO b2RlKCksIHdoaWNoIHZpc2l0cyBub2RlcyBpbiBwcmUgb3JkZXIsIHdlIG11c3Qgc3RhcnQgb3Vy IHNlYXJjaCBmcm9tIHRoZSBsYXN0IGRlc2NlbmRlbnQgbm9kZSwKKyAgICAgICAgd2hpY2ggaXMg dmlzaXRlZCB0cmF2ZXJzZU5leHROb2RlIGltbWVpZGF0bGV5IGJlZm9yZSByZWFjaGluZyB0aGUg cm9vdCBub2RlLgorCisgICAgICAgIFRlc3Q6IGZhc3QvZG9tL2h0bWxjb2xsZWN0aW9uLWJhY2t3 YXJkcy1zdWJ0cmVlLWl0ZXJhdGlvbi5odG1sCisKKyAgICAgICAgKiBodG1sL0hUTUxDb2xsZWN0 aW9uLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Omxhc3REZXNjZW5kZW50KToKKyAgICAgICAgKFdl YkNvcmUpOgorICAgICAgICAoV2ViQ29yZTo6aXRlbUJlZm9yZU9yQWZ0ZXIpOgorCiAyMDEyLTA3 LTE0ICBFcmljIENhcmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgogCiAgICAgICAgIEVu YWJsZSBBVkNGIGhhcmR3YXJlIHZpZGVvIGRlY29kaW5nCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9o dG1sL0hUTUxDb2xsZWN0aW9uLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9odG1s L0hUTUxDb2xsZWN0aW9uLmNwcAkocmV2aXNpb24gMTIyNjc3KQorKysgU291cmNlL1dlYkNvcmUv aHRtbC9IVE1MQ29sbGVjdGlvbi5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI0NiwxMyArMjQ2LDI5 IEBAIHN0YXRpYyBOb2RlKiBuZXh0Tm9kZShOb2RlKiBiYXNlLCBOb2RlKiAKICAgICAgICAgcmV0 dXJuIG9ubHlJbmNsdWRlRGlyZWN0Q2hpbGRyZW4gPyBwcmV2aW91cy0+cHJldmlvdXNTaWJsaW5n KCkgOiBwcmV2aW91cy0+dHJhdmVyc2VQcmV2aW91c05vZGUoYmFzZSk7CiB9CiAKK3N0YXRpYyBp bmxpbmUgTm9kZSogbGFzdERlc2NlbmRlbnQoTm9kZSogbm9kZSkKK3sKKyAgICBub2RlID0gbm9k ZS0+bGFzdENoaWxkKCk7CisgICAgZm9yIChOb2RlKiBjdXJyZW50ID0gbm9kZTsgY3VycmVudDsg Y3VycmVudCA9IGN1cnJlbnQtPmxhc3RDaGlsZCgpKQorICAgICAgICBub2RlID0gY3VycmVudDsK KyAgICByZXR1cm4gbm9kZTsKK30KKwogdGVtcGxhdGU8Ym9vbCBmb3J3YXJkPgogc3RhdGljIEVs ZW1lbnQqIGl0ZW1CZWZvcmVPckFmdGVyKENvbGxlY3Rpb25UeXBlIHR5cGUsIE5vZGUqIGJhc2Us IHVuc2lnbmVkJiBvZmZzZXRJbkFycmF5LCBOb2RlKiBwcmV2aW91cykKIHsKICAgICBBU1NFUlRf VU5VU0VEKG9mZnNldEluQXJyYXksICFvZmZzZXRJbkFycmF5KTsKICAgICBib29sIG9ubHlJbmNs dWRlRGlyZWN0Q2hpbGRyZW4gPSBzaG91bGRPbmx5SW5jbHVkZURpcmVjdENoaWxkcmVuKHR5cGUp OwogICAgIE5vZGUqIHJvb3ROb2RlID0gYmFzZTsKLSAgICBOb2RlKiBjdXJyZW50ID0gcHJldmlv dXMgPyBuZXh0Tm9kZTxmb3J3YXJkPihyb290Tm9kZSwgcHJldmlvdXMsIG9ubHlJbmNsdWRlRGly ZWN0Q2hpbGRyZW4pIDogKGZvcndhcmQgPyBiYXNlLT5maXJzdENoaWxkKCkgOiBiYXNlLT5sYXN0 Q2hpbGQoKSk7CisgICAgTm9kZSogY3VycmVudDsKKyAgICBpZiAocHJldmlvdXMpCisgICAgICAg IGN1cnJlbnQgPSBuZXh0Tm9kZTxmb3J3YXJkPihyb290Tm9kZSwgcHJldmlvdXMsIG9ubHlJbmNs dWRlRGlyZWN0Q2hpbGRyZW4pOworICAgIGVsc2UgeworICAgICAgICBpZiAoZm9yd2FyZCkKKyAg ICAgICAgICAgIGN1cnJlbnQgPSByb290Tm9kZS0+Zmlyc3RDaGlsZCgpOworICAgICAgICBlbHNl CisgICAgICAgICAgICBjdXJyZW50ID0gb25seUluY2x1ZGVEaXJlY3RDaGlsZHJlbiA/IHJvb3RO b2RlLT5sYXN0Q2hpbGQoKSA6IGxhc3REZXNjZW5kZW50KHJvb3ROb2RlKTsKKyAgICB9CiAKICAg ICBmb3IgKDsgY3VycmVudDsgY3VycmVudCA9IG5leHROb2RlPGZvcndhcmQ+KHJvb3ROb2RlLCBj dXJyZW50LCBvbmx5SW5jbHVkZURpcmVjdENoaWxkcmVuKSkgewogICAgICAgICBpZiAoY3VycmVu dC0+aXNFbGVtZW50Tm9kZSgpICYmIGlzQWNjZXB0YWJsZUVsZW1lbnQodHlwZSwgdG9FbGVtZW50 KGN1cnJlbnQpKSkKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBM YXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyMjY3NykKKysrIExheW91dFRlc3RzL0No YW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA3LTE0ICBSeW9z dWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFJFR1JFU1NJT04ocjEyMjY2 MCk6IENhbm5vdCBpdGVyYXRlIG92ZXIgSFRNTENvbGxlY3Rpb24gdGhhdCBjb250YWlucyBub24t Y2hpbGQgZGVzY2VuZGVudCBub2RlcyBpbiBzb21lIGNvbmRpdGlvbnMKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTkxMzM0CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIGEgcmVncmVzc2lvbiB0ZXN0LiBX aXRob3V0IHRoaXMgcGF0Y2gsIGl0IHJlc3VsdHMgaW4gYW4gY29uc29sZSBlcnJvciBpbiByZWxl YXNlIGJ1aWxkcyBhbmQgYW4gYXNzZXJ0aW9uIGZhaWx1cmUKKyAgICAgICAgaW4gZGVidWcgYnVp bGRzLgorCisgICAgICAgICogZmFzdC9kb20vaHRtbGNvbGxlY3Rpb24tYmFja3dhcmRzLXN1YnRy ZWUtaXRlcmF0aW9uLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICogZmFzdC9kb20vaHRt bGNvbGxlY3Rpb24tYmFja3dhcmRzLXN1YnRyZWUtaXRlcmF0aW9uLmh0bWw6IEFkZGVkLgorCiAy MDEyLTA3LTE0ICBSeW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgogCiAgICAgICAgIEFj Y2Vzc2luZyB0aGUgbGFzdCBpdGVtIGluIGNoaWxkcmVuIHNob3VsZCBiZSBhIGNvbnN0YW50IHRp bWUgb3BlcmF0aW9uCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2RvbS9odG1sY29sbGVjdGlvbi1i YWNrd2FyZHMtc3VidHJlZS1pdGVyYXRpb24tZXhwZWN0ZWQudHh0Cj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExh eW91dFRlc3RzL2Zhc3QvZG9tL2h0bWxjb2xsZWN0aW9uLWJhY2t3YXJkcy1zdWJ0cmVlLWl0ZXJh dGlvbi1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2RvbS9o dG1sY29sbGVjdGlvbi1iYWNrd2FyZHMtc3VidHJlZS1pdGVyYXRpb24tZXhwZWN0ZWQudHh0CShy ZXZpc2lvbiAwKQpAQCAtMCwwICsxLDQgQEAKK1Rlc3RzIHRoYXQgSFRNTENvbGxlY3Rpb24gb2Yg YSBzdWJ0cmVlIChhcyBzdXBwb3NlZCB0byBkaXJlY3QgY2hpbGRyZW4pIGNhbiBiZSBpdGVyYXRl ZCBiYWNrd2FyZHMuCitUaGVyZSBzaG91bGQgYmUgbm8gY29uc29sZSBlcnJvciBhbmQgV2ViS2l0 IHNob3VsZCBub3QgaGl0IGFuIGFzc2VydGlvbi4KKworUEFTUy4KSW5kZXg6IExheW91dFRlc3Rz L2Zhc3QvZG9tL2h0bWxjb2xsZWN0aW9uLWJhY2t3YXJkcy1zdWJ0cmVlLWl0ZXJhdGlvbi5odG1s Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL2h0bWxjb2xsZWN0aW9uLWJhY2t3 YXJkcy1zdWJ0cmVlLWl0ZXJhdGlvbi5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMv ZmFzdC9kb20vaHRtbGNvbGxlY3Rpb24tYmFja3dhcmRzLXN1YnRyZWUtaXRlcmF0aW9uLmh0bWwJ KHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMjEgQEAKKzwhRE9DVFlQRSBodG1sPgorPGh0bWw+Cis8 Ym9keT4KKzxpbWc+PGltZz48aW1nPjxpbWc+Cis8c2NyaXB0PgorCit2YXIgaW1hZ2VzID0gZG9j dW1lbnQuaW1hZ2VzOworZm9yICh2YXIgaSA9IGltYWdlcy5sZW5ndGg7IGkgPiAwOyBpLS0pIHsK KyAgICBpbWFnZXNbMF0uY2xhc3MgPSAnZm9vJzsKKyAgICBpbWFnZXNbaSAtIDFdLmNsYXNzID0g J2Zvbyc7Cit9CisKK2lmICh3aW5kb3cudGVzdFJ1bm5lcikKKyAgICB0ZXN0UnVubmVyLmR1bXBB c1RleHQoKTsKKworZG9jdW1lbnQuYm9keS5pbm5lckhUTUwgPSAnVGVzdHMgdGhhdCBIVE1MQ29s bGVjdGlvbiBvZiBhIHN1YnRyZWUgKGFzIHN1cHBvc2VkIHRvIGRpcmVjdCBjaGlsZHJlbikgY2Fu IGJlIGl0ZXJhdGVkIGJhY2t3YXJkcy48YnI+JworKyAnVGhlcmUgc2hvdWxkIGJlIG5vIGNvbnNv bGUgZXJyb3IgYW5kIFdlYktpdCBzaG91bGQgbm90IGhpdCBhbiBhc3NlcnRpb24uPGJyPjxicj5Q QVNTLic7CisKKzwvc2NyaXB0PgorPC9ib2R5PgorPC9odG1sPgo=