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

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Andrew Oakley, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 439199 [details]
Copy of test case at http://ado.is-a-geek.net/mozilla/nodefilter.html
(Assignee)

Comment 2

8 years ago
Which exception does Opera throw? INVALID_STATE_ERR?
(Assignee)

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

8 years ago
Created attachment 439212 [details] [diff] [review]
Opera approach
(Assignee)

Comment 4

8 years ago
That approach affects also to TreeWalker, which seems to have similar
undefined behaviors.
(Reporter)

Comment 5

8 years ago
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.
(Assignee)

Comment 6

8 years ago
I think throwing INVALID_STATE_ERR is the right thing to do yes.
But I'd like to see what sicking says about this.
(Assignee)

Comment 7

8 years ago
#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
Summary: Moving a NodeIterator from its NodeFilter causes a crash → Moving a NodeIterator from its NodeFilter causes a crash [@ nsNodeIterator::NodePointer::MoveForward]
Comment on attachment 439212 [details] [diff] [review]
Opera approach

Yeah, this seems like a sensible thing to do.
(Assignee)

Comment 9

8 years ago
Comment on attachment 439212 [details] [diff] [review]
Opera approach

I'll write still mochitests for this.
Attachment #439212 - Flags: review?(jonas)
Attachment #439212 - Flags: review?(jonas) → review+
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.
(Assignee)

Comment 11

8 years ago
Comment on attachment 439212 [details] [diff] [review]
Opera approach

This is not enough after all. There is another null pointer crash.
Attachment #439212 - Flags: review+ → review-
(Assignee)

Comment 12

8 years ago
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.
Attachment #439354 - Flags: review?(jonas)
Comment on attachment 439354 [details] [diff] [review]
v2 + tests

Good catch!
Attachment #439354 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c2e522dc68b4
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Assignee: nobody → Olli.Pettay
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.