Last Comment Bug 559526 - Moving a NodeIterator from its NodeFilter causes a crash [@ nsNodeIterator::NodePointer::MoveForward]
: Moving a NodeIterator from its NodeFilter causes a crash [@ nsNodeIterator::N...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
http://ado.is-a-geek.net/mozilla/node...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-15 02:42 PDT by Andrew Oakley
Modified: 2013-04-04 13:53 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Copy of test case at http://ado.is-a-geek.net/mozilla/nodefilter.html (1.15 KB, text/html)
2010-04-15 02:46 PDT, Andrew Oakley
no flags Details
Opera approach (2.62 KB, patch)
2010-04-15 04:47 PDT, Olli Pettay [:smaug]
bugs: review-
Details | Diff | Review
v2 + tests (6.19 KB, patch)
2010-04-15 14:35 PDT, Olli Pettay [:smaug]
jonas: review+
Details | Diff | Review

Description Andrew Oakley 2010-04-15 02:42:56 PDT
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 2010-04-15 02:46:31 PDT
Created attachment 439199 [details]
Copy of test case at http://ado.is-a-geek.net/mozilla/nodefilter.html
Comment 2 Olli Pettay [:smaug] 2010-04-15 04:40:56 PDT
Which exception does Opera throw? INVALID_STATE_ERR?
Comment 3 Olli Pettay [:smaug] 2010-04-15 04:47:31 PDT
Created attachment 439212 [details] [diff] [review]
Opera approach
Comment 4 Olli Pettay [:smaug] 2010-04-15 04:52:15 PDT
That approach affects also to TreeWalker, which seems to have similar
undefined behaviors.
Comment 5 Andrew Oakley 2010-04-15 06:13:12 PDT
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] 2010-04-15 06:30:24 PDT
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] 2010-04-15 06:45:54 PDT
#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) 2010-04-15 12:28:43 PDT
Comment on attachment 439212 [details] [diff] [review]
Opera approach

Yeah, this seems like a sensible thing to do.
Comment 9 Olli Pettay [:smaug] 2010-04-15 12:46:46 PDT
Comment on attachment 439212 [details] [diff] [review]
Opera approach

I'll write still mochitests for this.
Comment 10 Alex Vincent [:WeirdAl] 2010-04-15 13:20:38 PDT
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] 2010-04-15 14:03:53 PDT
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] 2010-04-15 14:35:17 PDT
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 13 Jonas Sicking (:sicking) 2010-04-15 15:19:30 PDT
Comment on attachment 439354 [details] [diff] [review]
v2 + tests

Good catch!
Comment 14 Olli Pettay [:smaug] 2010-04-16 13:53:45 PDT
http://hg.mozilla.org/mozilla-central/rev/c2e522dc68b4

Note You need to log in before you can comment on or make changes to this bug.