304514 - nsContentIterator::Init(range) doesn't consider Document nodes

Status: RESOLVED WORKSFORME

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

Description

[Alex Vincent [:WeirdAl]]

Derived from bug 157373.  A patch there fixes nsRange::ToString() for document
nodes, but in the process a similar bug in nsContentIterator causes an endless
loop with assertions.

###!!! ASSERTION: Null current node in an iterator that's not done!: 'mCurNode',
file c:/mozsource/mozilla/content/base/src/nsContentIterator.cpp, line 1191

nsContentIterator::Init() sets mCommonParent = do_QueryInterface(dN), where
mCommonParent is of type nsIContent and dN is a nsIDOMNode.  However, dN can be
a Document node, in which case the QI will return null.

Similar problems occur with StartCon a few lines later, resulting in the Init
function bailing out and returning NS_ERROR_FAILURE.  Naturally, the next frame
in the stack, nsRange::ToString(), didn't bother checking a return value:

iter->Init(this);

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRange.cpp#2094

A similar bug exists for EndCon, a few lines later.  I don't know whether or not
other failures exist here.

Steps to reproduce:
(1) Apply attachment 192565 [details] [diff] [review] to your tree.
(2) make tier_9
(3) Start your browser and point it to bug 157373.
(4) Click on the URL link in the bug.

Expected results:
Two alerts.

Actual results:
One alert, followed by the above assertion in an endless loop, and hang.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: work-in-progress

Here's what I have after a little thinking:

// nsContentIterator::Init(nsIDOMRange* aRange) has to set these.
  nsCOMPtr<nsIContent> mCurNode;
  nsCOMPtr<nsIContent> mFirst;
  nsCOMPtr<nsIContent> mLast;
  nsCOMPtr<nsIContent> mCommonParent;

/**
 * Flag for nsPreContentIterator.  
 */
  PRBool mPre;

/**
 * This array is cleared in Init(), and then populated in RebuildIndexStack.
 */
  nsAutoVoidArray mIndexes;

// Not changed in Init.
  PRInt32 mCachedIndex;

// False, except at end on Init, where it becomes !mCurNode.

This patch attempts to set mCurNode, mFirst and mLast accordingly when we have
a boundary point in the range as a document.

Unfortunately, mCurrentParent is still used quite heavily throughout the rest
of the iterator, and it will be null under these circumstances.  I don't have
any way of knowing how to fix the code for this.  RebuildIndexStack, for
instance, will eventually return NS_ERROR_FAILURE.  I shudder at the thought of
GetNextSibling, NextNode and friends.

Comment 2

[Alex Vincent [:WeirdAl]]

> // False, except at end on Init, where it becomes !mCurNode.
mIsDone

Comment 3

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 4

[Andrei Purice]

Hey Alex,
Is this issue still occurring for you or can we close it?

Comment 5

[Alex Vincent [:WeirdAl]]

I'd say close it. I have no idea what the state of the code is, but I haven't touched this in over a decade.

Comment 6

[Andrei Purice]

Marking this as Resolved > Worksforme based on the last comment.