15274 2007-09-24 14:11:16 -0700 REGRESSION: NameNodeList fails to invalidate length cache 2007-10-14 04:49:51 -0700 1 1 1 Unclassified WebKit DOM 523.x (Safari 3) Mac OS X 10.4 RESOLVED FIXED http://www.lptl.jussieu.fr/users/bernu/pub/fred.html HasReduction, InRadar, Regression P1 Normal --- 1 bernu webkit-unassigned ap mrowe oldest_to_newest 42 0 bernu 2007-09-24 14:11:16 -0700 I replace an element in Javascript by : y = cloneNode( x ) removeChild( x ) Then I try to access "y" using GetElementsByName, BUT I get an "undefined value" It looks like some pointers are not handled correctly. In the example, clicking one of the the lines lead to an error. 38 1 bernu 2007-09-24 14:33:13 -0700 sounds related to bug #8080 18 2 ddkilzer 2007-09-24 21:58:53 -0700 (In reply to comment #0) > I replace an element in Javascript by : > y = cloneNode( x ) > removeChild( x ) > > Then I try to access "y" using GetElementsByName, BUT I get an "undefined > value" Have you added "y" to the document when you try to use getElementsByName()? If not, it won't be found. Please post a test case. The URL on the bug seems to be inaccessible now: http://www.lptl.jussieu.fr/users/bernu/pub/fred.html Thanks! 24 3 mrowe 2007-09-24 23:34:36 -0700 The test case is accessible now, but after a quick look at it I'm not able to determine what to do to reproduce the described bugs. Clicking on the links has no visible effect. A minimal test case with clear instructions which makes clear what you consider to be the PASS/FAIL condition would be great! 11 4 bernu 2007-09-25 01:08:15 -0700 (In reply to comment #3) > The test case is accessible now, but after a quick look at it I'm not able to > determine what to do to reproduce the described bugs. Clicking on the links > has no visible effect. A minimal test case with clear instructions which makes > clear what you consider to be the PASS/FAIL condition would be great! > The messages appear in the console.log : Clicking on the link gives : my_a a-2 <DOMNodeList: 0x5387dc0> 2 <DOMNodeList: 0x4c0be0> 2 Undefined value line 21 The first 4 lines are ok (of course, thenode addreesses will be different each time) The last line shows the error. 2 5 16385 ddkilzer 2007-09-25 07:20:59 -0700 Created attachment 16385 Test case from URL 56996 6 sam 2007-09-25 08:55:46 -0700 The reason "undefined value" is being returned in the test case is that NodeLists indexing is 0 based, not 1 based. Therefore, xs[2] does not exist. 56997 7 bernu 2007-09-25 09:22:35 -0700 (In reply to comment #6) > The reason "undefined value" is being returned in the test case is that > NodeLists indexing is 0 based, not 1 based. Therefore, xs[2] does not exist. > Right. Sorry for posting this. I have a problem on a more complicated case and I thoughI found where was the problem. I go back to work more on it Sorry again for that 57028 8 bernu 2007-09-25 14:01:49 -0700 I think I found the real problem. (http://www.lptl.jussieu.fr/users/bernu/pub/fred.html) the Javascript is reduced to function del( ){ var divs = document.getElementsByName( "my_a" ) ; divs[0].parentNode.removeChild( divs[ 0 ] ) ; console.log( divs.length ) ; console.log( divs[0].id ) ; console.log( divs[1].id ) ; } and the HTML to <body> <div href="" id="a-1" name="my_a" >div 1</div> <div href="" id="a-2" name="my_a" >div 2</div> <input type="submit" onclick="del() ; return false;" /> </body> The result in the console is 2 a-1 Null value error on line accessing divs[1].id What I understand is : getElementsByName gives a list of 2 nodes. Removing the first div leaves the nodeList divs with 2 elements. Accessing the first element surprinsingly worked here (but might not in a more complicate case) Accessing the second element fails on Safari. On FireFox, the result is more consistent : 1 a-2 error divs[1] has no property. Thus, removing an element in a list is the problem that WebKit is not doing as other browsers. After removing divs[0], divs.length=1 57030 9 mrowe 2007-09-25 14:15:07 -0700 Bernard, can you please attach a standalone test case that matches what you describe? This will make it much easier to verify that we're all looking at the same thing. 57031 10 bernu 2007-09-25 14:18:50 -0700 (In reply to comment #9) > Bernard, can you please attach a standalone test case that matches what you > describe? This will make it much easier to verify that we're all looking at > the same thing. > I did put the following link at the top of my comment. I just verified the example is accessible http://www.lptl.jussieu.fr/users/bernu/pub/fred.html 57033 11 mrowe 2007-09-25 14:24:12 -0700 Sorry about that, I totally missed that link :) 57064 12 mjs 2007-09-26 04:11:26 -0700 I would guess removeChild is failing to invalidate the cache kept by the divs collection that getElementsByName returns. 57831 13 16541 ap 2007-10-05 04:20:35 -0700 Created attachment 16541 further reduced test case 57832 14 ap 2007-10-05 04:21:51 -0700 This is a regression from shipping Safari/WebKit. 57856 15 ddkilzer 2007-10-05 08:46:40 -0700 Note that this bug reproduces in WebKit nightly build r11976 (earliest nightly build available). 57857 16 ddkilzer 2007-10-05 08:52:46 -0700 <rdar://problem/5524756> 58054 17 eric 2007-10-07 19:14:22 -0700 My guess is this was hyatt's qname patch: http://trac.webkit.org/projects/webkit/changeset/9824 I don't see why rootNodeChildrenChanged: // Other methods (not part of DOM) virtual void rootNodeChildrenChanged() { } virtual void rootNodeAttributeChanged() { NodeList::rootNodeChildrenChanged(); } should be empty like that. I expect it really should invalidate its length cache, if nothing else. 58055 18 eric 2007-10-07 19:30:18 -0700 Well, it may still have been broken by the qname change (I'm unsure of that). That's where the rootNodeChildrenChanged() function came from as far as I can tell. However, it does not appear that rootNodeChildrenChanged() is the right function to call. As mjs suggested, it's simply that a node doesn't know to tell the document to invalidate its name caches when it's removed. A node removed anywhere in the tree should check its name and then notify the caches. A node whos name changes anywhere in the tree will need to do the same. It looks like bool EventTargetNode::dispatchSubtreeModifiedEvent(bool sendChildrenChanged) is the responsible function. Which isn't very smart about what nodes it notifies, it just does a blanket notification it seems. It looks like the current system indicates that the empty override for rootNodeChildrenChanged() should simply be removed (and that will fix the problem). There might be a more efficient solution to build however. 58057 19 16584 eric 2007-10-07 20:07:05 -0700 Created attachment 16584 a fix If this is approved for trunk, please someone else land it. If it's approved only for feature-branch, I'm happy to land the patch. 58433 20 16584 mjs 2007-10-13 18:17:29 -0700 Comment on attachment 16584 a fix r=me 58489 21 mrowe 2007-10-14 04:49:51 -0700 Landed in r26585 after adding a LayoutTest ChangeLog entry. 16385 2007-09-25 07:20:59 -0700 2007-09-25 07:20:59 -0700 Test case from URL fred.html text/html 1080 ddkilzer PCFET0NUWVBFIGh0bWwgUFVCTElDICItLy9XM0MvL0RURCBYSFRNTCAxLjAgVHJhbnNpdGlvbmFs Ly9FTiIgImh0dHA6Ly93d3cudzMub3JnL1RSLzIwMDAvUkVDLXhodG1sMS0yMDAwMDEyNi9EVEQv eGh0bWwxLXRyYW5zaXRpb25hbC5kdGQiPgo8aHRtbCB4bWxucz0iaHR0cDovL3d3dy53My5vcmcv MTk5OS94aHRtbCIgeG1sOmxhbmc9ImVuIiBsYW5nPSJlbiI+Cgk8aGVhZD4KCQk8bWV0YSBodHRw LWVxdWl2PSJjb250ZW50LXR5cGUiIGNvbnRlbnQ9InRleHQvaHRtbDtjaGFyc2V0PVVURi04IiAv PgoJCTx0aXRsZT5CdWcgb24gcmVtb3ZlQ2hpbGQgPyBpbnNlckJlZm9yZSA/IGdldEVsZW1lbnRz QnlOYW1lID88L3RpdGxlPgoJCTxzY3JpcHQgdHlwZT0idGV4dC9qYXZhc2NyaXB0Ij4KPCEtLQpm dW5jdGlvbiByZXBsYWNlX2EoIHggKXsKCWNvbnNvbGUubG9nKCB4Lm5hbWUgKyAiICAiICsgeC5p ZCApIDsKCXZhciB4cyA9IGRvY3VtZW50LmdldEVsZW1lbnRzQnlOYW1lKCAibXlfYSIgKSA7Cglj b25zb2xlLmxvZyggeHMgKSA7Cgljb25zb2xlLmxvZyggeHMubGVuZ3RoICkgOwoJdmFyIHkgPSB4 LmNsb25lTm9kZSggeCAsIHRydWUgKSA7Cgl4LmlkICs9ICJkdW0iIDsKCXkub25jbGljayA9IHgu b25jbGljayA7IAoJeC5wYXJlbnROb2RlLmluc2VydEJlZm9yZSggeSAsIHggKSA7Cgl4LnBhcmVu dE5vZGUucmVtb3ZlQ2hpbGQoIHggKSA7Cgl2YXIgeHMgPSBkb2N1bWVudC5nZXRFbGVtZW50c0J5 TmFtZSggIm15X2EiICkgOwoJY29uc29sZS5sb2coIHhzICkgOwoJY29uc29sZS5sb2coIHhzLmxl bmd0aCApIDsKCWNvbnNvbGUubG9nKCB4c1syXS50YWdOYW1lICkgOwoJCn0KLS0+Cjwvc2NyaXB0 PgoJPC9oZWFkPgoJPGJvZHk+CgkJPGEgaHJlZj0iIiBpZD0iYS0xIiBuYW1lPSJteV9hIiBvbmNs aWNrPSJyZXBsYWNlX2EoIHRoaXMgKSA7IHJldHVybiBmYWxzZSA7IiA+Y2xpY2sgMTwvYT48YnIg Lz4KCQk8YSBocmVmPSIiIGlkPSJhLTIiIG5hbWU9Im15X2EiIG9uY2xpY2s9InJlcGxhY2VfYSgg dGhpcyApIDsgcmV0dXJuIGZhbHNlIDsiID5jbGljayAyPC9hPgoJPC9ib2R5Pgo8L2h0bWw+ 16541 2007-10-05 04:20:35 -0700 2007-10-05 04:20:35 -0700 further reduced test case fred.html text/html 576 ap PGh0bWw+CjxoZWFkPgogIDx0aXRsZT5CdWcgb24gcmVtb3ZlQ2hpbGQgPyBpbnNlckJlZm9yZSA/ IGdldEVsZW1lbnRzQnlOYW1lID88L3RpdGxlPgo8L2hlYWQ+Cjxib2R5PgogIDxkaXYgaHJlZj0i IiBpZD0iYS0xIiBuYW1lPSJteV9hIj5kaXYgMTwvZGl2PgogIDxkaXYgaHJlZj0iIiBpZD0iYS0y IiBuYW1lPSJteV9hIj5kaXYgMjwvZGl2PgogIDxzY3JpcHQ+CiAgICB0cnkgewogICAgICB2YXIg ZGl2cyA9IGRvY3VtZW50LmdldEVsZW1lbnRzQnlOYW1lKCJteV9hIik7CiAgICAgIGRpdnNbMF0u cGFyZW50Tm9kZS5yZW1vdmVDaGlsZChkaXZzWzBdKTsKICAgICAgZG9jdW1lbnQud3JpdGUoIlNo b3VsZCBiZSAxOiAiICsgZGl2cy5sZW5ndGggKyAiPGJyPiIpOwogICAgICBkaXZzWzBdLnBhcmVu dE5vZGUucmVtb3ZlQ2hpbGQoZGl2c1swXSk7CiAgICAgIGRvY3VtZW50LndyaXRlKCJTaG91bGQg YmUgMDogIiArIGRpdnMubGVuZ3RoICsgIjxicj4iKTsKICAgIH0gY2F0Y2goZXgpIHsKICAgICAg ZG9jdW1lbnQud3JpdGUoZXggKyAiPGJyPiIpOwogICAgfQogIDwvc2NyaXB0Pgo8L2JvZHk+Cjwv aHRtbD4K 16584 2007-10-07 20:07:05 -0700 2007-10-13 18:17:29 -0700 a fix fix.patch text/plain 2723 eric SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNjEwMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDctMTAtMDcgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJraXQub3Jn PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJFR1JF U1NJT046IEphdmFzY3JpcHQgYnVnIGluIGdldEVsZW1lbnRzQnlOYW1lCisgICAgICAgIGh0dHA6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1Mjc0CisKKyAgICAgICAgVGVzdDog ZmFzdC9kb20vZ2V0ZWxlbWVudGJ5bmFtZS1pbnZhbGlkYXRpb24uaHRtbAorCisgICAgICAgICog ZG9tL05hbWVOb2RlTGlzdC5oOiByZW1vdmUgZW1wdHkgcm9vdE5vZGVDaGlsZHJlbkNoYW5nZWQg b3ZlcnJpZGUKKwogMjAwNy0xMC0wNyAgTmlrb2xhcyBaaW1tZXJtYW5uICA8emltbWVybWFubkBr ZGUub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVyaWMuCkluZGV4OiBXZWJDb3JlL2RvbS9O YW1lTm9kZUxpc3QuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2RvbS9OYW1lTm9kZUxpc3QuaAko cmV2aXNpb24gMjYxMDEpCisrKyBXZWJDb3JlL2RvbS9OYW1lTm9kZUxpc3QuaAkod29ya2luZyBj b3B5KQpAQCAtNDMsNyArNDMsNiBAQCBwdWJsaWM6CiAgICAgdmlydHVhbCBOb2RlKiBpdGVtKHVu c2lnbmVkIGluZGV4KSBjb25zdDsKIAogICAgIC8vIE90aGVyIG1ldGhvZHMgKG5vdCBwYXJ0IG9m IERPTSkKLSAgICB2aXJ0dWFsIHZvaWQgcm9vdE5vZGVDaGlsZHJlbkNoYW5nZWQoKSB7IH0KICAg ICB2aXJ0dWFsIHZvaWQgcm9vdE5vZGVBdHRyaWJ1dGVDaGFuZ2VkKCkgeyBOb2RlTGlzdDo6cm9v dE5vZGVDaGlsZHJlbkNoYW5nZWQoKTsgfQogCiBwcm90ZWN0ZWQ6CkluZGV4OiBMYXlvdXRUZXN0 cy9mYXN0L2RvbS9nZXRlbGVtZW50YnluYW1lLWludmFsaWRhdGlvbi1leHBlY3RlZC50eHQKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9kb20vZ2V0ZWxlbWVudGJ5bmFtZS1pbnZhbGlk YXRpb24tZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFzdC9kb20v Z2V0ZWxlbWVudGJ5bmFtZS1pbnZhbGlkYXRpb24tZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQpA QCAtMCwwICsxLDYgQEAKK2ZpeCBmb3IgYnVnIDE1Mjc0CitTaG91bGQgYmUgMTogMQorU2hvdWxk IGJlIDA6IDAKK1Nob3VsZCBiZSAxOiAxCitTaG91bGQgYmUgMDogMAorCkluZGV4OiBMYXlvdXRU ZXN0cy9mYXN0L2RvbS9nZXRlbGVtZW50YnluYW1lLWludmFsaWRhdGlvbi5odG1sCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL2dldGVsZW1lbnRieW5hbWUtaW52YWxpZGF0aW9u Lmh0bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2RvbS9nZXRlbGVtZW50Ynlu YW1lLWludmFsaWRhdGlvbi5odG1sCShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDI4IEBACis8aHRt bD4KKzxib2R5PgorICA8c3Bhbj5maXggZm9yIDxhIGhyZWY9Imh0dHA6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE1Mjc0Ij5idWcgMTUyNzQ8L2E+PC9zcGFuPjxicj4KKyAgPGRp diBuYW1lPSJteV9hIj5kaXYgMTwvZGl2PgorICA8ZGl2IG5hbWU9Im15X2EiPmRpdiAyPC9kaXY+ CisgIDxwPjwvcD4KKyAgPHA+PC9wPgorICA8c2NyaXB0PgorICAgIGlmICh3aW5kb3cubGF5b3V0 VGVzdENvbnRyb2xsZXIpCisgICAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7 CisgICAgdHJ5IHsKKyAgICAgIHZhciBkaXZzID0gZG9jdW1lbnQuZ2V0RWxlbWVudHNCeU5hbWUo Im15X2EiKTsKKyAgICAgIGRpdnNbMF0ucGFyZW50Tm9kZS5yZW1vdmVDaGlsZChkaXZzWzBdKTsK KyAgICAgIGRvY3VtZW50LndyaXRlKCJTaG91bGQgYmUgMTogIiArIGRpdnMubGVuZ3RoICsgIjxi cj4iKTsKKyAgICAgIGRpdnNbMF0ucGFyZW50Tm9kZS5yZW1vdmVDaGlsZChkaXZzWzBdKTsKKyAg ICAgIGRvY3VtZW50LndyaXRlKCJTaG91bGQgYmUgMDogIiArIGRpdnMubGVuZ3RoICsgIjxicj4i KTsKKyAgICAgIAorICAgICAgdmFyIGRpdnMgPSBkb2N1bWVudC5nZXRFbGVtZW50c0J5VGFnTmFt ZSgicCIpOworICAgICAgZGl2c1swXS5wYXJlbnROb2RlLnJlbW92ZUNoaWxkKGRpdnNbMF0pOwor ICAgICAgZG9jdW1lbnQud3JpdGUoIlNob3VsZCBiZSAxOiAiICsgZGl2cy5sZW5ndGggKyAiPGJy PiIpOworICAgICAgZGl2c1swXS5wYXJlbnROb2RlLnJlbW92ZUNoaWxkKGRpdnNbMF0pOworICAg ICAgZG9jdW1lbnQud3JpdGUoIlNob3VsZCBiZSAwOiAiICsgZGl2cy5sZW5ndGggKyAiPGJyPiIp OworICAgIH0gY2F0Y2goZXgpIHsKKyAgICAgIGRvY3VtZW50LndyaXRlKGV4ICsgIjxicj5GQUlM RUQiKTsKKyAgICB9CisgIDwvc2NyaXB0PgorPC9ib2R5PgorPC9odG1sPgo=