Closed Bug 302775 Opened 19 years ago Closed 16 years ago

extractContents doesn't work if start and end node of a Range object is an attribute node [@ nsContentSubtreeIterator::Init]

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: celelibi, Assigned: smaug)

References

()

Details

(Keywords: crash, testcase, verified1.8)

Crash Data

Attachments

(5 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)

Just lets me show you a bit of code :

<html>
  <head>
    <title>crashRange</title>
    <script type="text/javascript">
      function crash() {
        su = document.getElementsByTagName('input')[0].getAttributeNode('value');
        ran = document.createRange();
        ran.setStart(su, 0);
        ran.setEnd(su, 1);
        ran.extractContents(); // fx crashs on this line
      }
    </script>
  </head>
  <body onload="crash()">
    <input type="text" value="abcdefghij" />
  </body>
</html>

What to say more ?

In fact the bug is in nsContentIterator.cpp on line 1334 :
1331  // short circuit when start node == end node
1332  if (startParent == endParent)
1333  {
1334    cChild = cStartP->GetChildAt(0);

(notice that my Range shouldn't be collapsed)
The bug is that cStartP object is not good. its mRawPtr property contain 0x00
wherewas it should not.
The value of cStartP was that assigned a few lines earlier by the return of the
function do_QueryInterface.

Reproducible: Always

Steps to Reproduce:
1. Create a new Range
2. assign its boundary points like to select the value of an attribute.
3. Call extractContents. :)

Actual Results:  
Firefox crash. (seg fault)
It try to access the memory at the adress 0x00.
eax = 0x0;
mov edx, [eax];

Expected Results:  
It should return the content of the attribut selected.

firefox crash with any versions and on any OS.
OS: Linux → All
Summary: If start and end node of a Range object is an attribute node, extractContents will crash firefox. → JavaScript : If start and end node of a Range object is an attribute node, extractContents will crash firefox.
Yep, doesn't waste any time:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB7943093E
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730
Firefox/1.0+ ID:2005073004 
Assignee: nobody → traversal-range
Status: UNCONFIRMED → NEW
Component: General → DOM: Traversal-Range
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → Trunk
Using 7/28 debug build...here's where the crash occurs.

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRa
wPtr != 0', file ../../dist/include/xpcom\nsCOMPtr.h, line 849
Flags: blocking1.8b4?
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050729
Firefox/1.0+ ID:2005072921
Crashes me. Also crashes 1.06
Keywords: crash, testcase
Summary: JavaScript : If start and end node of a Range object is an attribute node, extractContents will crash firefox. → JavaScript : If start and end node of a Range object is an attribute node, extractContents will crash firefox. [@ nsContentSubtreeIterator::Init]
bz, any clues about this one?
when did this crash start happening? I don't see it in the topcrash reports. is
this severe?
Assignee: traversal-range → mrbkap
Flags: blocking1.8b4? → blocking1.8b4-
(In reply to comment #5)
> when did this crash start happening? I don't see it in the topcrash reports. is
> this severe?

I'd suspect this is a crash that's been in the code base for a very long time. 
Based on the description, it sounds pretty rare. Attributes are rarely a window
selection or a DOM range selection -- you have to go out of your way to get there.

I'm a bit surprised that the setEnd call didn't throw an exception.  DOM 3 Core
states that XML attributes may have child nodes (text or entity references), but
it says nothing about HTML attributes specifically having child nodes...

I see something familiar here.  We have a few bugs in nsContentIterator.cpp
where a DOM node is expected to QueryInterface to nsIContent.  Document nodes
don't implement that, and as my debugger indicates, neither do attribute nodes.
   Hence the crash, precisely as celelibi describes it.
Comment 6 is right on -- the start node of a range need not be an nsIContent,
and the content iterator assumes it is.
I've had this sitting around in my tree for a couple of days... This seems like
a reasonable wallpaper until we can figure out how to solve this problem where
this code assumes that everything in the world QIs to nsIContent.
We'll take crash defense patches that don't add risk.

/be
Flags: blocking1.8b4- → blocking1.8b4+
Comment on attachment 193193 [details] [diff] [review]
wallpaper over the crash

r+sr+a=me, go fast.

/be
Attachment #193193 - Flags: superreview+
Attachment #193193 - Flags: review+
Attachment #193193 - Flags: approval1.8b4+
I checked the wallpaper (attachment 193193 [details] [diff] [review]) in (with NS_ERROR_NOT_IMPLEMENTED
replaced with NS_ERROR_FAILURE) on both the trunk and MOZILLA_1_8_BRANCH.
Leaving this bug open so that we can explore a better solution (hopefully
returning the real contents!).
Severity: critical → normal
Keywords: fixed1.8
Summary: JavaScript : If start and end node of a Range object is an attribute node, extractContents will crash firefox. [@ nsContentSubtreeIterator::Init] → extractContents doesn't work if start and end node of a Range object is an attribute node [@ nsContentSubtreeIterator::Init]
This is fixed for 1.8b4/fx1.5, but left open for more complete fixes on the
1.9a1 trunk, so flagging that.

/be
Flags: blocking1.8b4+ → blocking1.9a1+
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8verified1.8
Blocks: acid3
Depends on: 332148
Attached patch WIP (obsolete) — Splinter Review
With bug 332148 gives one point in ACID3.
But the patch is really just WIP.
Patch is missing nsINodeIterator.h. Would it make sense to split of the range/iterator changes from the attribute node changes? The former look fairly straightforward, but the latter scare me a bit. I wonder how much code relies on GetNodeParent never returning an attribute node.
Yes, I'll probably split the patch.
Flags: wanted1.9.1+
Attached patch WIP2 (obsolete) — Splinter Review
No separate NodeIterator (we don't need yet another interface), but make
ContentIterator to handle nsINodes (the interface could be renamed to NodeIterator). This leads to some extra QIs in editor, but is still IMO the
right thing to do.
ACID3+1

Peter, what do you think about this approach? Didn't split the patch, but
the attribute stuff isn't really that complicated.
Comparing to the actually functionality changes the size of the patch is pretty
huge.
Attachment #320745 - Attachment is obsolete: true
Attachment #332418 - Flags: review?(peterv)
Comment on attachment 332418 [details] [diff] [review]
WIP2

peter is perhaps on vacation already, so jst, if you have time...
Attachment #332418 - Flags: review?(jst)
Smaug, does that patch also make things work when the range start and end parents are both the Document?
QA Contact: ian → traversal-range
Comment on attachment 332418 [details] [diff] [review]
WIP2

So, after applying a bunch of patches to see how we were doing on Acid3, counting patches sitting in bugs, I noticed that we were leaking content nodes on the Acid3 test.

I just narrowed the leak down to this patch.

That said, I don't know if the leak is due to a bug in this patch or a bug in code that this patch now allows to be executed when running Acid3.
At the very least, nsDOMAttribute::RemoveChildAt in this patch leaks, since mChild is a raw pointer that we manually addref, not an nsCOMPtr.
Attached patch V3Splinter Review
NS_RELEASE(mChild), fixes the leak, at least here.
Gives 2 points in ACID3.
Attachment #338638 - Flags: review?(peterv)
Attachment #332418 - Attachment is obsolete: true
Attachment #332418 - Flags: review?(peterv)
Attachment #332418 - Flags: review?(jst)
(In reply to comment #23)
> Gives 2 points in ACID3.
Tests 7 and 10
And now 3 points, 7,10 and 11.
And with bug 454325 this fixes also ACID3 test 9.
(In reply to comment #23)
> NS_RELEASE(mChild), fixes the leak, at least here.

It fixes the leak for me as well.
Crash when trying to build, following advices from bug 454325 :

It crashes when building nsDOMAttribute.cpp.

/home/fred/logs/fx2/src/content/base/src/nsDOMAttribute.cpp:59:24: erreur:
nsTextNode.h : Aucun fichier ou dossier de ce type

=> No such file or directory.

And it stops building :(
I found the answer. As my root source directory is called src and not mozilla, a new directory is created for nsTextNode.h.

Looking at the patch, I saw that :

+++ mozilla/content/base/src/nsTextNode.h	2008-09-15 16:43:33.000000000 +0300

Could it be "fixed" in anyway in order to avoid a file not found bug ?
You should be applying the patch from the root of the tree (not one dir above), with -p1, no?
Comment on attachment 338638 [details] [diff] [review]
V3

>diff --git a/content/base/public/nsIContentIterator.h b/content/base/public/nsIContentIterator.h

> #define NS_ICONTENTITERTOR_IID \

Want to make this NS_ICONTENTITERATOR_IID?

>@@ -631,17 +632,17 @@ nsContentIterator::GetNextSibling(nsINod

>-  nsIContent *sib = parent->GetChildAt(indx);
>+  nsINode* sib = parent->GetChildAt(indx);

Nit:
nsINode *sib

>@@ -124,19 +124,16 @@ nsRange::CompareNodeToRange(nsIContent* 

>     // end of the root node, becasue it has no parent.

While you're here:
s/becasue/because/

>+nsTextNode::BindToAttribute(nsIAttribute* aAttr)
>+{
>+  NS_ASSERTION(!IsInDoc(), "Unbind before binding!");
>+  NS_ASSERTION(!GetNodeParent(), "Unbind before binding!");
>+  NS_ASSERTION(HasSameOwnerDoc(aAttr), "Wrong owner document!");
>+
>+  mParentPtrBits = reinterpret_cast<PtrBits>(aAttr);
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsTextNode::UnbindFromAttribute()
>+{
>+  NS_ASSERTION(GetNodeParent(), "Bind before unbinging!");
>+  NS_ASSERTION(GetNodeParent() &&
>+               GetNodeParent()->IsNodeOfType(nsINode::eATTRIBUTE),
>+               "Use this method only to unbind from an attribute!");
>+  mParentPtrBits = 0;
>+  return NS_OK;
> }

Should these call nsNodeUtils::ParentChainChanged? If so, can we just use BindToTree/UnbindFromTree?

>@@ -203,17 +203,17 @@ nsQueryContentEventHandler::GenerateFlat

>-    nsIContent* content = iter->GetCurrentNode();
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(iter->GetCurrentNode());
>     if (!content)
>       continue;

Use IsNodeOfType(nsINode::eCONTENT) and cast.

>@@ -284,19 +284,19 @@ nsQueryContentEventHandler::SetRangeFrom

>-    content = iter->GetCurrentNode();
>+    content = do_QueryInterface(iter->GetCurrentNode());
>     if (!content)
>       continue;

Ditto.

>@@ -3839,17 +3840,17 @@ nsHTMLEditor::GetEmbeddedObjects(nsISupp

>-      nsIContent *content = iter->GetCurrentNode();
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(iter->GetCurrentNode());
>       nsCOMPtr<nsIDOMNode> node (do_QueryInterface(content));

Do we need content still? Otherwise drop it and just set node to do_QueryInterface(iter->GetCurrentNode()).

>@@ -4545,17 +4546,17 @@ nsHTMLEditor::CollapseAdjacentTextNodes(

>+    nsCOMPtr<nsIContent> content = do_QueryInterface(iter->GetCurrentNode());
> 
>     nsCOMPtr<nsIDOMCharacterData> text = do_QueryInterface(content);
>     nsCOMPtr<nsIDOMNode>          node = do_QueryInterface(content);

I think we only need text here.

>@@ -4280,30 +4280,31 @@ nsTextServicesDocument::GetFirstTextNode

>-  nsIContent *content = mIterator->GetCurrentNode();
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mIterator->GetCurrentNode());

Just change this to nsINode*?

>-    NS_ADDREF(*aContent = mIterator->GetCurrentNode());
>+    nsCOMPtr<nsIContent> current = do_QueryInterface(mIterator->GetCurrentNode());
>+    current.swap(*aContent);

Can you use CallQueryInterface?

>@@ -4314,30 +4315,31 @@ nsTextServicesDocument::GetFirstTextNode

>-  nsIContent *content = mIterator->GetCurrentNode();
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mIterator->GetCurrentNode());

Just change this to nsINode*?

>-    NS_ADDREF(*aContent = mIterator->GetCurrentNode());
>+    nsCOMPtr<nsIContent> current = do_QueryInterface(mIterator->GetCurrentNode());
>+    current.swap(*aContent);

Can you use CallQueryInterface?
Attachment #338638 - Flags: review?(peterv) → review+
> Should these call nsNodeUtils::ParentChainChanged? If so, can we just use
> BindToTree/UnbindFromTree?
Based on IRC, no.

> Can you use CallQueryInterface?
Would need a null check, so swap is easier.
Attached patch v4Splinter Review
Changed one dom1 test from todo to ok + other comments
Assignee: mrbkap → Olli.Pettay
Attachment #342147 - Flags: superreview?(jonas)
Comment on attachment 342147 [details] [diff] [review]
v4

So there are some details that are still missing for treating textnodes as children of attributes. We technically should allow textnodes (and cdata nodes) to be inserted as children of an attribute. Though we can do that if we ever decide to support having more than one child under attribute nodes.

I'm totally fine, or would even prefer to, wait with that.

>@@ -4545,23 +4545,20 @@ nsHTMLEditor::CollapseAdjacentTextNodes(
>   nsCOMPtr<nsIContentIterator> iter =
>     do_CreateInstance("@mozilla.org/content/subtree-content-iterator;1", &result);
>   if (NS_FAILED(result)) return result;
> 
>   iter->Init(aInRange);
> 
>   while (!iter->IsDone())
>   {
>-    nsIContent *content = iter->GetCurrentNode();  
>-
>-    nsCOMPtr<nsIDOMCharacterData> text = do_QueryInterface(content);
>-    nsCOMPtr<nsIDOMNode>          node = do_QueryInterface(content);
>-    if (text && node && IsEditable(node))
>-    {
>-      textNodes.AppendElement(node.get());
>+    nsCOMPtr<nsIDOMCharacterData> text = do_QueryInterface(iter->GetCurrentNode());
>+    if (text && IsEditable(text))
>+    {
>+      textNodes.AppendElement(text.get());
>     }

I think you can remove the .get().

Patch looks great!
Attachment #342147 - Flags: superreview?(jonas) → superreview+
(In reply to comment #34)
> (From update of attachment 342147 [details] [diff] [review])
> We technically should allow textnodes (and cdata nodes)
> to be inserted as children of an attribute.
Yes, insert/append/replace are still NS_ERROR_NOT_IMPLEMENTED.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsContentSubtreeIterator::Init]
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.