230840 - deCOMtaminate nsIDocumentObserver, nsIAttribute, nsIContentList, and nsIContentIterator.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

The above, and many many more, still need deCOMtaminating. Patch coming up that
knocks a few more interfaces off the list...

Comment 1

[Johnny Stenback (:jst)]

Attachment: deCOMtaminate.

Comment 2

[Johnny Stenback (:jst)]

Attachment: deCOMtaminate (diff -w of the above)

Comment 3

[Nicholas Allen]

What's up with the cygwin changes in the first patch?

Index: build/cygwin-wrapper
Index: nsprpub/build/cygwin-wrapper

Comment 4

[Johnny Stenback (:jst)]

Oops, sorry, those weren't supposed to be in this diff (I got them removed from
the -w one, but not the other one), ignore those.

Comment 5

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

Comment on attachment 138991 [details] [diff] [review]
deCOMtaminate (diff -w of the above)

>Index: content/base/src/nsContentIterator.cpp
>+NS_INTERFACE_MAP_BEGIN(nsContentIterator)
>+  NS_INTERFACE_MAP_ENTRY(nsIContentIterator)
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_INTERFACE_MAP_END

You could use NS_IMPL_QUERY_INTERFACE1 (or NS_IMPL_ISUPPORTS2 ?)

>-nsresult nsContentIterator::First()
>+void
>+nsContentIterator::First()
> {
>-  if (!mFirst)
>-    return NS_ERROR_FAILURE;
>+  if (mFirst) {
>+    nsresult rv = PositionAt(mFirst);
> 
>-  return PositionAt(mFirst);
>+    if (NS_FAILED(rv))
>+      mFirst = nsnull;
>+
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to position iterator!");
>+  }
>+
>+  mIsDone = mFirst == nsnull;
> }

Talked on irc about this code. If PositionAt really shouldn't be able to fail
here (i.e. it'll only fail if the node is outside the set of iterated nodes and
mFirst is always in that set) then remove the the |if(NS_FAILED| and just
assert.

You might also want to assert or warn if mFirst is null since that *should*
never happen.

Same for nsContentIterator::Last.

>-nsresult nsContentSubtreeIterator::First()
>+void
>+nsContentSubtreeIterator::First()
> {
>-  if (!mFirst) 
>-    return NS_ERROR_FAILURE;
>-  mIsDone = PR_FALSE;
>-  if (mFirst == mCurNode) 
>-    return NS_OK;
>+  mIsDone = mFirst == nsnull;
>+
>   mCurNode = mFirst;
>-  return NS_OK;
> }

Same here (and in Las), this changes the behaviour when mFirst is null.

>+void
>+nsContentSubtreeIterator::Next()
> {
>   if (mIsDone) 
>-    return NS_OK;
>+    return;
>   if (!mCurNode) 
>-    return NS_OK;
>+    return;

Feel free to combine these ifs.

>   if (mCurNode == mLast) 
>   {
>     mIsDone = PR_TRUE;
>-    return NS_OK;
>+    return;
>   }
>   
>   nsCOMPtr<nsIContent> nextNode;
>   if (NS_FAILED(GetNextSibling(mCurNode, address_of(nextNode), nsnull)))
>-    return NS_OK;
>+    return;
> /*
>   nextNode = GetDeepFirstChild(nextNode);
>   return GetTopAncestorInRange(nextNode, address_of(mCurNode));
> */
>   PRInt32 i = mEndNodes.IndexOf((void*)nextNode);
>   while (i != -1)
>   {
>     // as long as we are finding ancestors of the endpoint of the range,
>     // dive down into their children
>     nsIContent *cChild = nextNode->GetChildAt(0);
>     if (!cChild)
>-      return NS_ERROR_NULL_POINTER;
>+      return;
> 
>     // should be impossible to get a null pointer.  If we went all the 
>     // down the child chain to the bottom without finding an interior node, 
>     // then the previous node should have been the last, which was
>     // was tested at top of routine.

If that really shouldn't fail wouldn't an assertion be enough? (or shouldn't
there at least be an assertion in addition to returning now that it can't
return a fail-code)

>Index: content/base/src/nsDocument.h
>@@ -144,77 +144,106 @@ class nsDOMStyleSheetList : public nsIDO
> {
> public:
>   nsDOMStyleSheetList(nsIDocument *aDocument);
>   virtual ~nsDOMStyleSheetList();
> 
>   NS_DECL_ISUPPORTS
> 
>   NS_DECL_NSIDOMSTYLESHEETLIST
> 
>-  NS_IMETHOD BeginUpdate(nsIDocument *aDocument, nsUpdateType aUpdateType) {
>-    return NS_OK;
>+  virtual void BeginUpdate(nsIDocument *aDocument, nsUpdateType aUpdateType)
>+  {
>   }

Feel free to make this use the macros in nsIDocumentObserver.h (hrm, i thought
i hunted all these down when i first created those macros)

>Index: content/base/src/nsGeneratedIterator.cpp
> NS_IMPL_ADDREF(nsGeneratedContentIterator)
> NS_IMPL_RELEASE(nsGeneratedContentIterator)
...
>+NS_INTERFACE_MAP_BEGIN(nsGeneratedContentIterator)
>+  NS_INTERFACE_MAP_ENTRY(nsIGeneratedContentIterator)
>+  NS_INTERFACE_MAP_ENTRY(nsIContentIterator)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentIterator)
>+NS_INTERFACE_MAP_END

Could use NS_IMPL_ISUPPORTS2 here.

>-nsresult nsGeneratedContentIterator::First()
>+void
>+nsGeneratedContentIterator::First()
> {
>+  mIsDone = mFirst == nsnull;
>+
>   if (!mFirst) 
>-    return NS_ERROR_FAILURE;
>-  mIsDone = PR_FALSE;
>+    return;
>+

This does the !mFirst test twice, feel free to do |mIsDone = PR_TRUE| inside
the |if| instead.

Same for nsGeneratedContentIterator::Last.

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>@@ -990,24 +989,25 @@ nsHTMLEditor::NextNodeInBlock(nsIDOMNode
...
>+    node = do_QueryInterface(iter->GetCurrentNode());
>+    if (node && IsTextOrElementNode(node) && (node != blockParent) &&
>+        (node != aNode))

Please remove the unneccesary parenthesis.


I'm at nsHTMLEditor::TabInTable. More might follow

Comment 6

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

Comment on attachment 138991 [details] [diff] [review]
deCOMtaminate (diff -w of the above)

>Index: content/base/src/nsGeneratedIterator.cpp
>@@ -639,20 +615,24 @@ nsresult nsGeneratedContentIterator::Get
> nsresult nsGeneratedContentIterator::NextNode(nsCOMPtr<nsIContent> *ioNextNode)
> {
>   if (!ioNextNode)
>     return NS_ERROR_NULL_POINTER;
>     
>   if (mGenIter)
>   {
>     if (mGenIter->IsDone())
>       mGenIter = 0;
>-    else
>-       return mGenIter->Next();
>+    else {
>+      mGenIter->Next();
>+
>+      return NS_OK;
>+    }
>+

Don't you need to do something like:

mGenIter->Next();
return mGenIter->IsDone() ? NS_ERROR_FAILURE : NS_OK;

here? In general i wonder if it was a good idea to make Next() not return an
error, or at least a bool? Though i'm still pondering it.

Comment 7

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

Comment on attachment 138991 [details] [diff] [review]
deCOMtaminate (diff -w of the above)

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>@@ -1415,36 +1415,37 @@ NS_IMETHODIMP nsHTMLEditor::TabInTable(P
>   if (NS_FAILED(res)) return res;
>   if (!iter) return NS_ERROR_NULL_POINTER;
>   nsCOMPtr<nsIContent> cTbl = do_QueryInterface(tbl);
>   nsCOMPtr<nsIContent> cBlock = do_QueryInterface(cellElement);
>   res = iter->Init(cTbl);
>   if (NS_FAILED(res)) return res;
>   // position iter at block
>   res = iter->PositionAt(cBlock);
>   if (NS_FAILED(res)) return res;
>+
>   nsCOMPtr<nsIDOMNode> node;
>-  nsCOMPtr<nsIContent> cNode;
>   do
>   {
>-    if (inIsShift) res = iter->Prev();
>-    else res = iter->Next();
>-    if (NS_FAILED(res)) break;
>-    res = iter->CurrentNode(getter_AddRefs(cNode));
>-    if (NS_FAILED(res)) break;
>-    node = do_QueryInterface(cNode);
>+    if (inIsShift)
>+      iter->Prev();
>+    else
>+      iter->Next();
>+
>+    node = do_QueryInterface(iter->GetCurrentNode());

you need to add something like |if (iter->IsDone()) break;| before calling
GetCurrentNode() here afaict.

>@@ -4740,48 +4726,47 @@ nsHTMLEditor::CollapseAdjacentTextNodes(
> 
>   // build a list of editable text nodes
>   nsresult result;
>   nsCOMPtr<nsIContentIterator> iter =
>     do_CreateInstance("@mozilla.org/content/subtree-content-iterator;1", &result);
>   if (NS_FAILED(result)) return result;
>   if (!iter) return NS_ERROR_NULL_POINTER;
> 
>   iter->Init(aInRange);
>-  nsCOMPtr<nsIContent> content;
>-  result = iter->CurrentNode(getter_AddRefs(content));  
>-  if (!content) return NS_OK;

You need to either do |if (iter->IsDone()) return NS_OK;| or init result to
NS_OK at the top. Oh how i hate code that ends with |return rv|.

>Index: editor/txtsvc/src/nsFilteredContentIterator.cpp
> nsFilteredContentIterator::Last()
...
>-  nsCOMPtr<nsIContent> currentContent;
>-  rv = mCurrentIterator->CurrentNode(getter_AddRefs(currentContent));
>+  nsIContent *currentContent = mCurrentIterator->GetCurrentNode();
>   nsCOMPtr<nsIDOMNode> node(do_QueryInterface(currentContent));

You can remove |currentContent| entierly here.

>@@ -4014,42 +3908,36 @@ nsTextServicesDocument::GetUncollapsedSe
...
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>+      content = iter->GetCurrentNode();
> 
>       if (NS_FAILED(result))
>         return result;
> 
>       if (!content)
>         return NS_ERROR_FAILURE;

You need to remove these two |if|s.

Also i'm in general worried about the amounts of places where you remove
|result = ...| stuff since there are plenty of code that ends with |return
result;| where result wasn't set when it was declared. When you check this
patch in, keep a close eye on tinderbox to see if number of warnings goes up
and check for variable-used-before-set warnings. (You might even want to try to
check in during a low-checkin period to make sure that noone removes warnings
elsewhere at the same time).

This is why it's so evil to have code that ends with |return result;|, i've ran
into this problem before and although it's not difficult to find it might give
bugs in very unexpected areas.

> nsTextServicesDocument::FirstTextNodeInCurrentBlock(nsIContentIterator *iter)
...
>-  while (NS_ENUMERATOR_FALSE == iter->IsDone())
>+  while (!iter->IsDone())
>   {
>-  	result = iter->CurrentNode(getter_AddRefs(content));
>-
>-    if (NS_FAILED(result))
>-      return result;
>+  	content = iter->GetCurrentNode();

Please fix the tab here since you're chaning that line anyway.

>@@ -4393,64 +4260,55 @@ nsTextServicesDocument::FirstTextNodeInP
>   // current block:
> 
>   result = FirstTextNodeInCurrentBlock(aIterator);
> 
>   if (NS_FAILED(result))
>     return NS_ERROR_FAILURE;
> 
>   // Point mIterator to the first node before the first text node:
> 
>-  result = aIterator->Prev();
>-
>-  if (NS_FAILED(result))
>-    return result;
>+  aIterator->Prev();
> 
>   // Now find the first text node of the next block:
> 
>   return FirstTextNodeInCurrentBlock(aIterator);

Do you need to do |if (aIterator->IsDone()) return NS_ERROR_FAILURE;| here?

> nsTextServicesDocument::FirstTextNodeInNextBlock(nsIContentIterator *aIterator)
...
>-  while (NS_ENUMERATOR_FALSE == aIterator->IsDone())
>+  while (!aIterator->IsDone())
>   {
>-  	result = aIterator->CurrentNode(getter_AddRefs(content));
>-    if (NS_FAILED(result))
>-      return result;
>+  	content = aIterator->GetCurrentNode();

Tab.

>Index: embedding/components/find/src/nsFind.cpp
>@@ -373,30 +373,30 @@ nsFind::NextNode(nsIDOMRange* aSearchRan
>       printf("Setting initial offset to %d\n", mIterOffset);
> #endif
>       return NS_OK;
>     }
>   }
> 
>   while (1)
>   {
>     if (mFindBackward)
>-      rv = mIterator->Prev();
>+      mIterator->Prev();
>     else
>-      rv = mIterator->Next();
>-    if (NS_FAILED(rv)) break;
>-    rv = mIterator->CurrentNode(getter_AddRefs(content));
>+      mIterator->Next();
>+
>+    content = mIterator->GetCurrentNode();
> #ifdef DEBUG_FIND
>     nsCOMPtr<nsIDOMNode> dnode (do_QueryInterface(content));
>     printf(":::::: Got another node "); DumpNode(dnode);
> #endif
>     // nsIContentIterator.h says Next() will return error at end,
>     // but it doesn't really, so we have to check:
>-    if (NS_FAILED(rv) || !content)
>+    if (!content)
>       break;

The comment is wrong since Next() doesn't return an error any more. You
probably also want to do this check before the #ifdef.

Comment 8

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

Comment on attachment 138991 [details] [diff] [review]
deCOMtaminate (diff -w of the above)

>Index: editor/txtsvc/src/nsTextServicesDocument.cpp
>@@ -1386,51 +1330,42 @@ nsTextServicesDocument::LastSelectedBloc
...
>-      while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+      while (!iter->IsDone())
>       {
>-        result = iter->CurrentNode(getter_AddRefs(content));
>+        content = iter->GetCurrentNode();
> 
>         if (NS_FAILED(result))
>         {
>           UNLOCK_DOC(this);
>           return result;
>         }
>         
>         if (!content)
>         {
>           UNLOCK_DOC(this);
>           return NS_ERROR_FAILURE;
>         }

Remvoe these two |if|s.

>@@ -1515,31 +1450,25 @@ nsTextServicesDocument::LastSelectedBloc
...
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>+      content = iter->GetCurrentNode();
> 
>       if (NS_FAILED(result))
>       {
>         UNLOCK_DOC(this);
>         return result;
>       }

Remove this |if|

>@@ -1657,29 +1583,23 @@ nsTextServicesDocument::LastSelectedBloc
...
>-  while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+  while (!iter->IsDone())
>   {
>-    result = iter->CurrentNode(getter_AddRefs(content));
>+    content = iter->GetCurrentNode();
> 
>     if (NS_FAILED(result))
>     {
>       UNLOCK_DOC(this);
>       return result;
>     }

Same same.

>@@ -2152,19 +2069,19 @@ nsTextServicesDocument::DeleteSelection(
> 
>       nsCOMPtr<nsIContent> curContent;
> 
>       if (mIteratorStatus != nsTextServicesDocument::eIsDone)
>       {
>         // The old iterator is still pointing to something valid,
>         // so get it's current node so we can restore it after we
>         // create the new iterator!
> 
>-        result = mIterator->CurrentNode(getter_AddRefs(curContent));
>+        curContent = mIterator->GetCurrentNode();
> 
>         if (NS_FAILED(result))
>         {
>           UNLOCK_DOC(this);
>           return result;
>         }
>       }

And again.

>@@ -3037,19 +2940,19 @@ nsTextServicesDocument::AdjustContentIte
> {
>   nsCOMPtr<nsIContent> content;
>   nsCOMPtr<nsIDOMNode> node;
>   nsresult result;
>   PRInt32 i;
> 
>   if (!mIterator)
>     return NS_ERROR_FAILURE;
> 
>-  result = mIterator->CurrentNode(getter_AddRefs(content));
>+  content = mIterator->GetCurrentNode();
> 
>   if (NS_FAILED(result))
>     return result;
> 
>   if (!content)
>     return NS_ERROR_FAILURE;

more of the same

>@@ -3679,41 +3582,38 @@ nsTextServicesDocument::GetCollapsedSele
>       return result;
> 
>     saveNode = parent;
>   }
> 
>   // Now iterate to the left, towards the beginning of
>   // the text block, to find the first text node you
>   // come across.
> 
>-  while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+  while (!iter->IsDone())
>   {
>-    result = iter->CurrentNode(getter_AddRefs(content));
>+    content = iter->GetCurrentNode();
> 
>     if (NS_FAILED(result))
>       return result;

I'm starting to see a pattern here...

>@@ -3733,41 +3633,38 @@ nsTextServicesDocument::GetCollapsedSele
>     // for a text node.
> 
>     content = do_QueryInterface(saveNode);
> 
>     result = iter->PositionAt(content);
> 
>     if (NS_FAILED(result))
>       return result;
> 
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>+      content = iter->GetCurrentNode();
> 
>       if (NS_FAILED(result))
>         return result;

Need I say?

>@@ -3982,30 +3879,27 @@ nsTextServicesDocument::GetUncollapsedSe
...
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>+      content = iter->GetCurrentNode();
> 
>       if (NS_FAILED(result))
>         return result;
> 
>       if (!content)
>         return NS_ERROR_FAILURE;

well.. y'know.

>@@ -4393,64 +4260,55 @@ nsTextServicesDocument::FirstTextNodeInP
...
> nsTextServicesDocument::FirstTextNodeInNextBlock(nsIContentIterator *aIterator)
...
>-  while (NS_ENUMERATOR_FALSE == aIterator->IsDone())
>+  while (!aIterator->IsDone())
>   {
>-  	result = aIterator->CurrentNode(getter_AddRefs(content));
>-    if (NS_FAILED(result))
>-      return result;
>+  	content = aIterator->GetCurrentNode();
> 
>     if (!content)
>       return NS_ERROR_FAILURE;

If i were wrong on all these it would be really embarrassing...

>@@ -4588,24 +4432,21 @@ nsTextServicesDocument::CreateOffsetTabl
...
>-  while (NS_ENUMERATOR_FALSE == aIterator->IsDone())
>+  while (!aIterator->IsDone())
>   {
>-    result = aIterator->CurrentNode(getter_AddRefs(content));
>-
>-    if (NS_FAILED(result))
>-      return result;
>+    content = aIterator->GetCurrentNode();
> 
>     if (!content)
>       return NS_ERROR_FAILURE;

...Though if you are wrong on all of them it'd be kind'a embarrassing for you
;-)

with that, r=me

However it feel like the patch is fairly risky, especially for editor so please
test around a bit there and it'd be great to have this in 1.7a

Comment 9

[Johnny Stenback (:jst)]

Attachment: Updated diff

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated diff -w

Comment 11

[Boris Zbarsky [:bzbarsky]]

I may not get to this for a bit (pretty swamped with non-mozilla work, and this
patch is _huge_).

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 139677 [details] [diff] [review]
Updated diff -w

>Index: content/base/public/nsIAttribute.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsIAttribute.h,v
>retrieving revision 1.2
>diff -u -9 -p -w -r1.2 nsIAttribute.h
>--- content/base/public/nsIAttribute.h	13 Jun 2003 20:07:32 -0000	1.2
>+++ content/base/public/nsIAttribute.h	22 Jan 2004 07:48:46 -0000
>@@ -1,10 +1,10 @@
>-/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* ***** BEGIN LICENSE BLOCK *****
>  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>  *
>  * The contents of this file are subject to the Mozilla Public License Version
>  * 1.1 (the "License"); you may not use this file except in compliance with
>  * the License. You may obtain a copy of the License at
>  * http://www.mozilla.org/MPL/
>  *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>@@ -48,18 +48,18 @@ class nsINodeInfo;
> #define NS_IATTRIBUTE_IID  \
>  {0xa6cf90dd, 0x15b3, 0x11d2,        \
>  {0x93, 0x2e, 0x00, 0x80, 0x5f, 0x8a, 0xdd, 0x32}}
> 
> class nsIAttribute : public nsISupports
> {
> public:
>   NS_DEFINE_STATIC_IID_ACCESSOR(NS_IATTRIBUTE_IID)
> 
>-  NS_IMETHOD DropReference() = 0;
>+  virtual void DropReference() = 0;
> 
>-  NS_IMETHOD SetContent(nsIContent* aContent) = 0;
>-  NS_IMETHOD GetContent(nsIContent** aContent) = 0;
>+  virtual void SetContent(nsIContent* aContent) = 0;
>+  virtual nsIContent *GetContent() = 0;
> 
>-  NS_IMETHOD GetNodeInfo(nsINodeInfo** aNodeInfo) = 0;
>+  virtual nsINodeInfo *GetNodeInfo() = 0;
> };

It looks like it's an error for an nsDOMAttribute to be initialized with a null
NodeInfo, so maybe this should just be NodeInfo() ? 

(I'm assuming there must be a case where it can legitimately be initialized
with a null nsIContent, since it's null checked.. but if not, same applies for
that)

I'm up to the beginning of nsContentList.cpp

Comment 13

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

nsDOMAttribute should be able to not hold an nsIContent. Don't know if it's the
case now, but it should in the future, so don't change the name for that one. I
agree on NodeInfo though.

Comment 14

[Johnny Stenback (:jst)]

Yeah, agreed. I'll change that. I thought I had a reason for not doing that
earlier, but I can't think of what that would have been.

Comment 15

[Johnny Stenback (:jst)]

Changed locally, and while I was at it, I moved mContent and mNodeInfo from
nsDOMAttribute to nsIAttribute, and inlined the getters/setters.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 139677 [details] [diff] [review]
Updated diff -w

>Index: content/base/src/nsRange.cpp
>===================================================================
>@@ -1534,39 +1506,34 @@ nsresult nsRange::DeleteContents()
> 
>   if (iter.IsDone())
>   {
>     // There's nothing for us to delete.
>     return CollapseRangeAfterDelete(this);
>   }
> 
>   // We delete backwards to avoid iterator problems!
> 
>-  res = iter.Last();
>-  if (NS_FAILED(res)) return res;
>-
>-  nsCOMPtr<nsIDOMNode> node;
>-  res = iter.CurrentNode(getter_AddRefs(node));
>-  if (NS_FAILED(res)) return res;
>-  if (!node) return NS_ERROR_FAILURE;
>+  iter.Last();
> 
>   PRBool handled = PR_FALSE;
> 
>   // With the exception of text nodes that contain one of the range
>   // end points, the subtree iterator should only give us back subtrees
>   // that are completely contained between the range's end points.
> 
>-  while (node)
>+  while (!iter.IsDone())
>   {
>+    nsCOMPtr<nsIDOMNode> node(iter.GetCurrentNode());
>+

Why does this need to keep a reference?

>@@ -1828,45 +1787,37 @@ nsresult nsRange::CloneContents(nsIDOMDo
>   if (iter.IsDone())
>   {
>     // There's nothing to add to the doc frag, we must be done!
> 
>     *aReturn = clonedFrag;
>     NS_IF_ADDREF(*aReturn);
>     return NS_OK;
>   }
> 
>-  res = iter.First();
>-  if (NS_FAILED(res)) return res;
>-
>-  nsCOMPtr<nsIDOMNode> node;
>-  res = iter.CurrentNode(getter_AddRefs(node));
>-  if (NS_FAILED(res)) return res;
>-  if (!node) return NS_ERROR_FAILURE;
>-
>+  iter.First();
> 
>   // With the exception of text nodes that contain one of the range
>   // end points, the subtree iterator should only give us back subtrees
>   // that are completely contained between the range's end points.
>   //
>   // Unfortunately these subtrees don't contain the parent hierarchy/context
>   // that the Range spec requires us to return. This loop clones the
>   // parent hierarchy, adds a cloned version of the subtree, to it, then
>   // correctly places this new subtree into the doc fragment.
> 
>-  while (node)
>+  while (!iter.IsDone())
>   {
>+    nsCOMPtr<nsIDOMNode> node(iter.GetCurrentNode());

Same here.

>@@ -1925,42 +1876,37 @@ nsresult nsRange::CloneContents(nsIDOMDo
> 
>     // Place the cloned subtree into the cloned doc frag tree!
> 
>     if (closestAncestor)
>     {
>       // Append the subtree under closestAncestor since it is the
>       // immediate parent of the subtree.
> 
>       res = closestAncestor->AppendChild(clone, getter_AddRefs(tmpNode));
>-
>-      if (NS_FAILED(res)) return res;
>     }
>     else
>     {
>       // If we get here, there is no missing parent hierarchy between 
>       // commonAncestor and node, so just append clone to commonCloneAncestor.
> 
>       res = commonCloneAncestor->AppendChild(clone, getter_AddRefs(tmpNode));
>-
>-      if (NS_FAILED(res)) return res;
>     }
>+    if (NS_FAILED(res)) return res;
> 
>     // Get the next subtree to be processed. The idea here is to setup
>     // the parameters for the next iteration of the loop.
> 
>-    res = iter.Next();
>+    iter.Next();
> 
>     if (iter.IsDone())
>       break; // We must be done!
> 
>-    nsCOMPtr<nsIDOMNode> nextNode;
>-    res = iter.CurrentNode(getter_AddRefs(nextNode));
>-    if (NS_FAILED(res)) return res;
>+    nsCOMPtr<nsIDOMNode> nextNode(iter.GetCurrentNode());

And here.


>@@ -2215,25 +2160,25 @@ nsresult nsRange::ToString(nsAString& aR
>   /* complex case: cStart != cEnd, or cStart not a text node
>      revisit - there are potential optimizations here and also tradeoffs.
>   */
> 
>   nsCOMPtr<nsIContentIterator> iter;
>   NS_NewContentIterator(getter_AddRefs(iter));
>   iter->Init(this);
>   
>   nsString tempString;
>-  nsCOMPtr<nsIContent> cN;
>  
>   // loop through the content iterator, which returns nodes in the range in 
>   // close tag order, and grab the text from any text node
>-  iter->CurrentNode(getter_AddRefs(cN));
>-  while (cN && (NS_ENUMERATOR_FALSE == iter->IsDone()))
>+  while (!iter->IsDone())
>   {
>+    nsCOMPtr<nsIContent> cN = iter->GetCurrentNode();
>+

And here.


>Index: editor/txtsvc/src/nsTextServicesDocument.cpp
>===================================================================
>@@ -858,51 +852,30 @@ nsTextServicesDocument::FirstSelectedBlo
> 
>       result = CreateContentIterator(range, getter_AddRefs(iter));
> 
>       if (NS_FAILED(result))
>       {
>         UNLOCK_DOC(this);
>         return result;
>       }
> 
>-      result = iter->Last();
>-
>-      if (NS_FAILED(result))
>-      {
>-        UNLOCK_DOC(this);
>-        return result;
>-      }
>-
>-      while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>-      {
>-        result = iter->CurrentNode(getter_AddRefs(content));
>-
>-        if (NS_FAILED(result))
>-        {
>-          UNLOCK_DOC(this);
>-          return result;
>-        }
>+      iter->Last();
>         
>-        if (!content)
>+      while (!iter->IsDone())
>         {
>-          UNLOCK_DOC(this);
>-          return NS_ERROR_FAILURE;
>-        }
>+        content = iter->GetCurrentNode();

Same question here.

>@@ -987,37 +960,25 @@ nsTextServicesDocument::FirstSelectedBlo
> 
>     result = CreateContentIterator(range, getter_AddRefs(iter));
> 
>     if (NS_FAILED(result))
>     {
>       UNLOCK_DOC(this);
>       return result;
>     }
> 
>-    result = iter->First();
>-
>-    if (NS_FAILED(result))
>-    {
>-      UNLOCK_DOC(this);
>-      return result;
>-    }
>+    iter->First();
> 
>     // Now walk through the range till we find a text node.
> 
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>-
>-      if (NS_FAILED(result))
>-      {
>-        UNLOCK_DOC(this);
>-        return result;
>-      }
>+      content = iter->GetCurrentNode();

Ditto.

>@@ -1129,35 +1087,23 @@ nsTextServicesDocument::FirstSelectedBlo
> 
>   result = CreateContentIterator(range, getter_AddRefs(iter));
> 
>   if (NS_FAILED(result))
>   {
>     UNLOCK_DOC(this);
>     return result;
>   }
> 
>-  result = iter->Last();
>-
>-  if (NS_FAILED(result))
>-  {
>-    UNLOCK_DOC(this);
>-    return result;
>-  }
>-
>-  while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>-  {
>-    result = iter->CurrentNode(getter_AddRefs(content));
>+  iter->Last();
> 
>-    if (NS_FAILED(result))
>+  while (!iter->IsDone())
>     {
>-      UNLOCK_DOC(this);
>-      return result;
>-    }
>+    content = iter->GetCurrentNode();

Yep.


and the same for LastSelectedBlock().
>@@ -2152,25 +2045,19 @@ nsTextServicesDocument::DeleteSelection(
> 
>       nsCOMPtr<nsIContent> curContent;
> 
>       if (mIteratorStatus != nsTextServicesDocument::eIsDone)
>       {
>         // The old iterator is still pointing to something valid,
>         // so get it's current node so we can restore it after we
>         // create the new iterator!
> 
>-        result = mIterator->CurrentNode(getter_AddRefs(curContent));
>-
>-        if (NS_FAILED(result))
>-        {
>-          UNLOCK_DOC(this);
>-          return result;
>-        }
>+        curContent = mIterator->GetCurrentNode();
>       }

Here too.

>@@ -2572,40 +2459,32 @@ nsTextServicesDocument::DeleteNode(nsIDO
>   if (!hasEntry)
>   {
>     // It's okay if the node isn't in the offset table, the
>     // editor could be cleaning house.
> 
>     UNLOCK_DOC(this);
>     return NS_OK;
>   }
> 
>-  nsCOMPtr<nsIContent> content;
>-  nsCOMPtr<nsIDOMNode> node;
>-  
>-  result = mIterator->CurrentNode(getter_AddRefs(content));
>-
>-  if (content)
>-  {
>-    node = do_QueryInterface(content);
>+  nsCOMPtr<nsIContent> content = mIterator->GetCurrentNode();
>+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(content);
> 
And here.

>@@ -2766,25 +2645,19 @@ nsTextServicesDocument::JoinNodes(nsIDOM
>   nsCOMPtr<nsIContent> leftContent = do_QueryInterface(aLeftNode);
>   nsCOMPtr<nsIContent> rightContent = do_QueryInterface(aRightNode);
> 
>   if (!leftContent || !rightContent)
>   {
>     UNLOCK_DOC(this);
>     return NS_ERROR_FAILURE;
>   }
> 
>-  result = mIterator->CurrentNode(getter_AddRefs(currentContent));
>-
>-  if (NS_FAILED(result))
>-  {
>-    UNLOCK_DOC(this);
>-    return result;
>-  }
>+  currentContent = mIterator->GetCurrentNode();

Ditto.

>@@ -3031,28 +2904,25 @@ nsTextServicesDocument::CreateDocumentCo
> 
>   return result;
> }
> 
> nsresult
> nsTextServicesDocument::AdjustContentIterator()
> {
>   nsCOMPtr<nsIContent> content;
>   nsCOMPtr<nsIDOMNode> node;
>-  nsresult result;
>+  nsresult result = NS_OK;
>   PRInt32 i;
> 
>   if (!mIterator)
>     return NS_ERROR_FAILURE;
> 
>-  result = mIterator->CurrentNode(getter_AddRefs(content));
>-
>-  if (NS_FAILED(result))
>-    return result;
>+  content = mIterator->GetCurrentNode();
> 

Ditto.

>@@ -3679,41 +3549,35 @@ nsTextServicesDocument::GetCollapsedSele
>       return result;
> 
>     saveNode = parent;
>   }
> 
>   // Now iterate to the left, towards the beginning of
>   // the text block, to find the first text node you
>   // come across.
> 
>-  while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+  while (!iter->IsDone())
>   {
>-    result = iter->CurrentNode(getter_AddRefs(content));
>-
>-    if (NS_FAILED(result))
>-      return result;
>+    content = iter->GetCurrentNode();

Same.

>@@ -3733,41 +3597,35 @@ nsTextServicesDocument::GetCollapsedSele
>     // for a text node.
> 
>     content = do_QueryInterface(saveNode);
> 
>     result = iter->PositionAt(content);
> 
>     if (NS_FAILED(result))
>       return result;
> 
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>-
>-      if (NS_FAILED(result))
>-        return result;
>+      content = iter->GetCurrentNode();

Same.

>@@ -3982,80 +3840,59 @@ nsTextServicesDocument::GetUncollapsedSe
> 
>   if (NS_FAILED(result))
>     return result;
> 
>   // Find the first text node in the range.
>   
>   PRBool found;
>   nsCOMPtr<nsIContent> content;
> 
>-  result = iter->First();
>- 
>-  if (NS_FAILED(result))
>-    return result;
>+  iter->First();
> 
>   if (! IsTextNode(p1))
>   {
>     found = PR_FALSE;
> 
>-    while (iter->IsDone() == NS_ENUMERATOR_FALSE)
>+    while (!iter->IsDone())
>     {
>-      result = iter->CurrentNode(getter_AddRefs(content));
>-
>-      if (NS_FAILED(result))
>-        return result;
>-
>-      if (!content)
>-        return NS_ERROR_FAILURE;
>+      content = iter->GetCurrentNode();

Same.


> nsresult
> nsTextServicesDocument::FirstTextNodeInCurrentBlock(nsIContentIterator *iter)
> {
>-  nsresult result;
>-
>   if (!iter)
>     return NS_ERROR_NULL_POINTER;
> 
>   ClearDidSkip(iter);
> 
>   nsCOMPtr<nsIContent> content;
>   nsCOMPtr<nsIContent> last;
> 
>   // Walk backwards over adjacent text nodes until
>   // we hit a block boundary:
> 
>-  while (NS_ENUMERATOR_FALSE == iter->IsDone())
>+  while (!iter->IsDone())
>   {
>-  	result = iter->CurrentNode(getter_AddRefs(content));
>-
>-    if (NS_FAILED(result))
>-      return result;
>+    content = iter->GetCurrentNode();

Here too.


> nsresult
> nsTextServicesDocument::FirstTextNodeInNextBlock(nsIContentIterator *aIterator)
> {
>   nsCOMPtr<nsIContent> content;
>   nsCOMPtr<nsIContent> prev;
>   PRBool crossedBlockBoundary = PR_FALSE;
>-  nsresult result;
> 
>   if (!aIterator)
>     return NS_ERROR_NULL_POINTER;
> 
>   ClearDidSkip(aIterator);
> 
>-  while (NS_ENUMERATOR_FALSE == aIterator->IsDone())
>+  while (!aIterator->IsDone())
>   {
>-  	result = aIterator->CurrentNode(getter_AddRefs(content));
>-    if (NS_FAILED(result))
>-      return result;
>-
>-    if (!content)
>-      return NS_ERROR_FAILURE;
>+    content = aIterator->GetCurrentNode();

Ditto.

sr=bryner with those changes.

Comment 17

[Harshal Pradhan]

Attachment: Camino bustage fix

I checked this in to fix the red on Camino tinderboxen from jst's checkin.

Comment 18

[Johnny Stenback (:jst)]

Thanks! Marking Fixed now.

Comment 19

[Aleksey Nogin]

This have added a "might be used uninitialized" warning to brad TBox:

+editor/txtsvc/src/nsTextServicesDocument.cpp:2046
+ `nsIContent*curContent' might be used uninitialized in this function

Indeed, it appears that curContent needs to be initialized to NULL.

Comment 20

[Johnny Stenback (:jst)]

Ah, thanks. Fix checked in.

Comment 21

[Scott MacGregor]

any chance this could have caused the content iterator in nsTextServicesDocument
to stop iterating over text nodes? The spell checker now hangs in an infinite
loop because of that and it started the morning after this was checked in which
was why I ask. See Bug #232343

Comment 22

[Boris Zbarsky [:bzbarsky]]

This looks like it's causing bug 236270 too...