User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:184.108.40.206) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729) Build Identifier: "Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a5pre) Gecko/20100414 Minefield/3.7a5pre" and "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:220.127.116.11) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)" See linked test case - the filter for a NodeIterator moves the iterator when it is called. This is a very stupid thing to do, but it should be fixed as it is a crash. Given this test case, Chrome and Safari stop iterating with no error, Opera throws an exception when the filter tries to move the iterator, IE does not support DOM Traversal. I think the Opera solution is probably the most sensible of these options - don't allow recursive calls to the iterator. This doesn't seem to be mentioned in the spec, I'll probably ask on webapps or www-dom (and update this bug as appropriate). Reproducible: Always
Created attachment 439199 [details] Copy of test case at http://ado.is-a-geek.net/mozilla/nodefilter.html
Which exception does Opera throw? INVALID_STATE_ERR?
That approach affects also to TreeWalker, which seems to have similar undefined behaviors.
Yes, opera does throw INVALID_STATE_ERR. I assume that you agree that this is the correct thing to do given that you have created a patch? Or is it just that its the easiest thing to implement? I should have probably mentioned TreeWalker, I havn't tested it but I imagine it is exactly the same as NodeIterator.
I think throwing INVALID_STATE_ERR is the right thing to do yes. But I'd like to see what sicking says about this.
#0 0x000000327b2a4d1d in nanosleep () from /lib64/libc.so.6 #1 0x000000327b2a4b90 in sleep () from /lib64/libc.so.6 #2 0x00007f8a4bc04d1d in ah_crap_handler (signum=11) at /home/smaug/mozilla/mozilla_cvs/hg/src/toolkit/xre/nsSigHandlers.cpp:164 #3 0x00007f8a4bc07d5c in nsProfileLock::FatalSignalHandler (signo=11, info=<value optimized out>, context=<value optimized out>) at nsProfileLock.cpp:221 #4 <signal handler called> #5 0x00007f8a3f8e9348 in nsNodeIterator::NodePointer::MoveForward (this=0x407dbf8, aRoot=0x407c430, aParent=0x0, aChildNum=-1) at /home/smaug/mozilla/mozilla_cvs/hg/src/content/base/src/nsNodeIterator.cpp:148 #6 0x00007f8a3f8e9fa3 in nsNodeIterator::NextOrPrevNode(int (nsNodeIterator::NodePointer::*)(nsINode*), nsIDOMNode**) () from /home/smaug/mozilla/mozilla_cvs/hg/src/ff_build/dist/bin/components/libgklayout.so #7 0x00007f8a43698485 in nsIDOMNodeIterator_NextNode (cx=0x3559cd0, argc=<value optimized out>, vp=0x2c6c728) at dom_quickstubs.cpp:19993 #8 0x00007f8a4b80c821 in js_Interpret (cx=<value optimized out>) at /home/smaug/mozilla/mozilla_cvs/hg/src/js/src/jsops.cpp:2236
Comment on attachment 439212 [details] [diff] [review] Opera approach Yeah, this seems like a sensible thing to do.
Comment on attachment 439212 [details] [diff] [review] Opera approach I'll write still mochitests for this.
Is this something we can land on branches, after suitable bake-time on trunk? I accidentally crashed FF3.5.9 stock by clicking on the testcase.
Comment on attachment 439212 [details] [diff] [review] Opera approach This is not enough after all. There is another null pointer crash.
Created attachment 439354 [details] [diff] [review] v2 + tests Because of the strange NodePointer and its handling, nsNodeIterator::NextOrPrevNode needs to check that we're not in mInAcceptNode. We must not change mWorkingPointer if we're already filtering something: when rv = TestNode(testNode, &filtered); NS_ENSURE_SUCCESS(rv, rv); returns, we clear mWorkingPointer, and in the earlier-in-the-stack-NextOrPrevNode call the code still assumes mWorkingPointer is valid. nsTreeWalker doesn't have similar problems.
Comment on attachment 439354 [details] [diff] [review] v2 + tests Good catch!