559526 - Moving a NodeIterator from its NodeFilter causes a crash [@ nsNodeIterator::NodePointer::MoveForward]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew Oakley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.3) 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:1.9.2.3) 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

Comment 1

[Andrew Oakley]

Attachment: Copy of test case at http://ado.is-a-geek.net/mozilla/nodefilter.html

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Which exception does Opera throw? INVALID_STATE_ERR?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Opera approach

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

That approach affects also to TreeWalker, which seems to have similar
undefined behaviors.

Comment 5

[Andrew Oakley]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I think throwing INVALID_STATE_ERR is the right thing to do yes.
But I'd like to see what sicking says about this.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

#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 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 439212 [details] [diff] [review]
Opera approach

Yeah, this seems like a sensible thing to do.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 439212 [details] [diff] [review]
Opera approach

I'll write still mochitests for this.

Comment 10

[Alex Vincent [:WeirdAl]]

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 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 439212 [details] [diff] [review]
Opera approach

This is not enough after all. There is another null pointer crash.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 439354 [details] [diff] [review]
v2 + tests

Good catch!

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/c2e522dc68b4