Bug 16758 – ASSERT when using TreeWalker methods for a current node outside of the root (Acid3)

• Component: DOM
• Status: RESOLVED FIXED

[Sam Weinig]

Running the Acid3 test in debug builds reveals an assertion failure:

ASSERTION FAILED: !stayWithin || !n->nextSibling() || n->nextSibling()->isDescendantOf(stayWithin)
(/Users/weinig/Code/webkit/OpenSource/WebCore/dom/Node.cpp:490 WebCore::Node* WebCore::Node::traverseNextNode(const WebCore::Node*) const)

This stems from the TreeWalker class being able to set the currentNode to an arbitrary node but not checking that is within it's root node when using the traverseNextNode/traversePreviousNode functions.

[Sam Weinig]

Created attachment 18291 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 18291 [details]
patch

Looks great!

[Sam Weinig]

Landed in r29203.

[Darin Adler]

Comment on attachment 18291 [details]
patch

This is wrong.

If you read the TreeWalker documentation, you'll see that it has well defined behavior when used on nodes outside the original subtree. The code correctly implemented most of those rules before -- this patch broke it!

I have a patch that fixes it. The test case expects behavior that is incorrect, so I need to change it around.

[Darin Adler]

Here is the relevant quote from the specification:

"All the TreeWalker's navigation methods operate in terms of the context of the currentNode at the time they are invoked, no matter what has happened to, or around, that node since the last time the TreeWalker was accessed. This remains true even if the currentNode is moved out of its original subtree."

And:

"If we instead insert the currentNode into the <subtree/> element, like so: [...] we have moved the currentNode out from under the TreeWalker's root node. This does not invalidate the TreeWalker; it may still be used to navigate relative to the currentNode."

The real fix here would be to remove the assertions, and I have done this in my new patch.

[Darin Adler]

The patch in bug 3492 addresses this.

[Sam Weinig]

I realized after landing that this was not correct (but only now thought that the right corse of action would have been to roll it out).  A fix I made is in bug 16759, but is probably not necessary with Darin's patch.