Bug 91334 – REGRESSION(r122660): Cannot iterate over HTMLCollection that contains non-child descendent nodes in some conditions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

REGRESSION(r122660): Cannot iterate over HTMLCollection that contains non-child descendent nodes in some conditions

[Ryosuke Niwa]

Created attachment 152449 [details]
Patch

[Ryosuke Niwa]

My attempt to unify HTMLCollection & DynamicNodeList revealed a regression.

[Ojan Vafai]

Comment on attachment 152449 [details]
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.

[Ryosuke Niwa]

Comment on attachment 152449 [details]
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.

[Ryosuke Niwa]

Committed r122689: <http://trac.webkit.org/changeset/122689>

[Ojan Vafai]

(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.

[Ryosuke Niwa]

(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.

[Ojan Vafai]

Comment on attachment 152449 [details]
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.