Last Comment Bug 302775 - extractContents doesn't work if start and end node of a Range object is an attribute node [@ nsContentSubtreeIterator::Init]
: extractContents doesn't work if start and end node of a Range object is an at...
Status: RESOLVED FIXED
: crash, testcase, verified1.8
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 All
: -- normal with 7 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
http://celelibi.no-ip.org/pub/crashRa...
Depends on: 332148
Blocks: acid3
  Show dependency treegraph
 
Reported: 2005-07-30 07:41 PDT by celelibi
Modified: 2013-04-04 13:53 PDT (History)
31 users (show)
mtschrep: wanted1.9.1+
dbaron: blocking1.9-
brendan: blocking1.9a1+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase from comment 0 (478 bytes, text/html)
2005-07-30 10:11 PDT, Steve England [:stevee]
no flags Details
wallpaper over the crash (1.32 KB, patch)
2005-08-19 12:14 PDT, Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017)
brendan: review+
brendan: superreview+
brendan: approval1.8b4+
Details | Diff | Splinter Review
WIP (54.99 KB, patch)
2008-05-13 10:51 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP2 (81.13 KB, patch)
2008-08-05 14:42 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
V3 (81.15 KB, patch)
2008-09-15 07:01 PDT, Olli Pettay [:smaug]
peterv: review+
Details | Diff | Splinter Review
v4 (83.49 KB, patch)
2008-10-07 15:09 PDT, Olli Pettay [:smaug]
jonas: superreview+
Details | Diff | Splinter Review
without .get() (83.48 KB, patch)
2008-10-15 02:52 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description celelibi 2005-07-30 07:41:36 PDT
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.
Comment 1 Ria Klaassen (not reading all bugmail) 2005-07-30 09:27:32 PDT
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 
Comment 2 Aaron Slunt 2005-07-30 10:03:17 PDT
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
Comment 3 Steve England [:stevee] 2005-07-30 10:11:27 PDT
Created attachment 191060 [details]
testcase from comment 0

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
Comment 4 Caleb 2005-08-04 02:13:25 PDT
bz, any clues about this one?
Comment 5 Asa Dotzler [:asa] 2005-08-04 15:19:35 PDT
when did this crash start happening? I don't see it in the topcrash reports. is
this severe?
Comment 6 Alex Vincent [:WeirdAl] 2005-08-15 16:21:18 PDT
(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 7 Boris Zbarsky [:bz] (still a bit busy) 2005-08-16 13:01:18 PDT
Comment 6 is right on -- the start node of a range need not be an nsIContent,
and the content iterator assumes it is.
Comment 8 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2005-08-19 12:14:48 PDT
Created attachment 193193 [details] [diff] [review]
wallpaper over the crash

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.
Comment 9 Brendan Eich [:brendan] 2005-08-23 12:39:09 PDT
We'll take crash defense patches that don't add risk.

/be
Comment 10 Brendan Eich [:brendan] 2005-08-23 12:40:42 PDT
Comment on attachment 193193 [details] [diff] [review]
wallpaper over the crash

r+sr+a=me, go fast.

/be
Comment 11 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2005-08-23 14:07:31 PDT
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!).
Comment 12 Brendan Eich [:brendan] 2005-08-26 19:03:41 PDT
This is fixed for 1.8b4/fx1.5, but left open for more complete fixes on the
1.9a1 trunk, so flagging that.

/be
Comment 13 Bob Clary [:bc:] 2005-11-10 08:57:10 PST
no crash firefox 1.5 rc2 winxp/linux
Comment 14 Alex Vincent [:WeirdAl] 2006-01-25 13:59:09 PST
Reference http://wiki.mozilla.org/DOM:ContentIterator_Issues .
Comment 15 Olli Pettay [:smaug] 2008-05-13 10:51:52 PDT
Created attachment 320745 [details] [diff] [review]
WIP

With bug 332148 gives one point in ACID3.
But the patch is really just WIP.
Comment 16 Peter Van der Beken [:peterv] 2008-05-17 03:52:44 PDT
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.
Comment 17 Olli Pettay [:smaug] 2008-05-17 11:46:30 PDT
Yes, I'll probably split the patch.
Comment 18 Olli Pettay [:smaug] 2008-08-05 14:42:31 PDT
Created attachment 332418 [details] [diff] [review]
WIP2

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.
Comment 19 Olli Pettay [:smaug] 2008-08-13 13:07:14 PDT
Comment on attachment 332418 [details] [diff] [review]
WIP2

peter is perhaps on vacation already, so jst, if you have time...
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2008-08-20 22:00:40 PDT
Smaug, does that patch also make things work when the range start and end parents are both the Document?
Comment 21 David Baron :dbaron: ⌚️UTC-10 2008-09-10 10:22:06 PDT
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.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2008-09-10 11:36:16 PDT
At the very least, nsDOMAttribute::RemoveChildAt in this patch leaks, since mChild is a raw pointer that we manually addref, not an nsCOMPtr.
Comment 23 Olli Pettay [:smaug] 2008-09-15 07:01:33 PDT
Created attachment 338638 [details] [diff] [review]
V3

NS_RELEASE(mChild), fixes the leak, at least here.
Gives 2 points in ACID3.
Comment 24 Olli Pettay [:smaug] 2008-09-15 07:18:34 PDT
(In reply to comment #23)
> Gives 2 points in ACID3.
Tests 7 and 10
Comment 25 Olli Pettay [:smaug] 2008-09-16 01:32:49 PDT
And now 3 points, 7,10 and 11.
Comment 26 Olli Pettay [:smaug] 2008-09-17 11:22:56 PDT
And with bug 454325 this fixes also ACID3 test 9.
Comment 27 David Baron :dbaron: ⌚️UTC-10 2008-09-17 13:34:12 PDT
(In reply to comment #23)
> NS_RELEASE(mChild), fixes the leak, at least here.

It fixes the leak for me as well.
Comment 28 Frederic Bezies 2008-09-17 23:02:45 PDT
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 :(
Comment 29 Frederic Bezies 2008-09-18 00:35:28 PDT
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 ?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2008-09-18 05:04:10 PDT
You should be applying the patch from the root of the tree (not one dir above), with -p1, no?
Comment 31 Peter Van der Beken [:peterv] 2008-10-07 12:23:18 PDT
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?
Comment 32 Olli Pettay [:smaug] 2008-10-07 13:55:55 PDT
> 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.
Comment 33 Olli Pettay [:smaug] 2008-10-07 15:09:48 PDT
Created attachment 342147 [details] [diff] [review]
v4

Changed one dom1 test from todo to ok + other comments
Comment 34 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-15 00:16:24 PDT
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!
Comment 35 Olli Pettay [:smaug] 2008-10-15 02:32:05 PDT
(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.
Comment 36 Olli Pettay [:smaug] 2008-10-15 02:52:03 PDT
Created attachment 343200 [details] [diff] [review]
without .get()
Comment 37 :Ms2ger (⌚ UTC+1/+2) 2010-08-23 10:21:35 PDT
http://hg.mozilla.org/mozilla-central/rev/165c867e30ed
Comment 38 :Ms2ger (⌚ UTC+1/+2) 2011-08-09 02:10:20 PDT
http://hg.mozilla.org/mozilla-central/rev/4b28aee8d473

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