Bug 15274

Summary: REGRESSION: NameNodeList fails to invalidate length cache
Product: WebKit Reporter: Bernard Bernu <bernu>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, mrowe
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.lptl.jussieu.fr/users/bernu/pub/fred.html
Attachments:
Description Flags
Test case from URL
none
further reduced test case
none
a fix mjs: review+

Bernard Bernu
Reported 2007-09-24 14:11:16 PDT
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.
Attachments
Test case from URL (1.05 KB, text/html)
2007-09-25 07:20 PDT, David Kilzer (:ddkilzer) 		no flags
Details
further reduced test case (576 bytes, text/html)
2007-10-05 04:20 PDT, Alexey Proskuryakov 		no flags
Details
a fix (2.66 KB, patch)
2007-10-07 20:07 PDT, Eric Seidel (no email) 		mjs: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Bernard Bernu
Comment 1 2007-09-24 14:33:13 PDT
sounds related to bug #8080
David Kilzer (:ddkilzer)
Comment 2 2007-09-24 21:58:53 PDT
(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!
Mark Rowe (bdash)
Comment 3 2007-09-24 23:34:36 PDT
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!
Bernard Bernu
Comment 4 2007-09-25 01:08:15 PDT
(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.
David Kilzer (:ddkilzer)
Comment 5 2007-09-25 07:20:59 PDT
Created attachment 16385 [details] Test case from URL
Sam Weinig
Comment 6 2007-09-25 08:55:46 PDT
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.
Bernard Bernu
Comment 7 2007-09-25 09:22:35 PDT
(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
Bernard Bernu
Comment 8 2007-09-25 14:01:49 PDT
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
Mark Rowe (bdash)
Comment 9 2007-09-25 14:15:07 PDT
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.
Bernard Bernu
Comment 10 2007-09-25 14:18:50 PDT
(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
Mark Rowe (bdash)
Comment 11 2007-09-25 14:24:12 PDT
Sorry about that, I totally missed that link :)
Maciej Stachowiak
Comment 12 2007-09-26 04:11:26 PDT
I would guess removeChild is failing to invalidate the cache kept by the divs collection that getElementsByName returns.
Alexey Proskuryakov
Comment 13 2007-10-05 04:20:35 PDT
Created attachment 16541 [details] further reduced test case
Alexey Proskuryakov
Comment 14 2007-10-05 04:21:51 PDT
This is a regression from shipping Safari/WebKit.
David Kilzer (:ddkilzer)
Comment 15 2007-10-05 08:46:40 PDT
Note that this bug reproduces in WebKit nightly build r11976 (earliest nightly build available).
David Kilzer (:ddkilzer)
Comment 16 2007-10-05 08:52:46 PDT
<rdar://problem/5524756>
Eric Seidel (no email)
Comment 17 2007-10-07 19:14:22 PDT
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.
Eric Seidel (no email)
Comment 18 2007-10-07 19:30:18 PDT
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.
Eric Seidel (no email)
Comment 19 2007-10-07 20:07:05 PDT
Created attachment 16584 [details] 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.
Maciej Stachowiak
Comment 20 2007-10-13 18:17:29 PDT
Comment on attachment 16584 [details] a fix r=me
Mark Rowe (bdash)
Comment 21 2007-10-14 04:49:51 PDT
Landed in r26585 after adding a LayoutTest ChangeLog entry.
Note You need to log in before you can comment on or make changes to this bug.