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

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: celelibi, Assigned: smaug)

Tracking

({crash, testcase, verified1.8})

Trunk
x86
All
crash, testcase, verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
blocking1.9 -
blocking1.9a1 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

12 years ago
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 

Updated

12 years ago
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

Comment 2

12 years ago
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

Updated

12 years ago
Flags: blocking1.8b4?
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

Updated

12 years ago
Keywords: crash, testcase

Updated

12 years ago
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]

Comment 4

12 years ago
bz, any clues about this one?

Comment 5

12 years ago
when did this crash start happening? I don't see it in the topcrash reports. is
this severe?

Updated

12 years ago
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.
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.
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+

Comment 13

12 years ago
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8 → verified1.8
Reference http://wiki.mozilla.org/DOM:ContentIterator_Issues .
Flags: blocking1.9-

Updated

10 years ago
Blocks: 410460
(Assignee)

Updated

9 years ago
Depends on: 332148
(Assignee)

Comment 15

9 years ago
Created attachment 320745 [details] [diff] [review]
WIP

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

Comment 17

9 years ago
Yes, I'll probably split the patch.

Updated

9 years ago
Flags: wanted1.9.1+
(Assignee)

Comment 18

9 years ago
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.
Attachment #320745 - Attachment is obsolete: true
Attachment #332418 - Flags: review?(peterv)
(Assignee)

Comment 19

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

Comment 23

9 years ago
Created attachment 338638 [details] [diff] [review]
V3

NS_RELEASE(mChild), fixes the leak, at least here.
Gives 2 points in ACID3.
Attachment #338638 - Flags: review?(peterv)
(Assignee)

Updated

9 years ago
Attachment #332418 - Attachment is obsolete: true
Attachment #332418 - Flags: review?(peterv)
Attachment #332418 - Flags: review?(jst)
(Assignee)

Comment 24

9 years ago
(In reply to comment #23)
> Gives 2 points in ACID3.
Tests 7 and 10
(Assignee)

Comment 25

9 years ago
And now 3 points, 7,10 and 11.
(Assignee)

Comment 26

9 years ago
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.

Comment 28

9 years ago
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

9 years ago
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+
(Assignee)

Comment 32

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

Comment 33

9 years ago
Created attachment 342147 [details] [diff] [review]
v4

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+
(Assignee)

Comment 35

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

Comment 36

9 years ago
Created attachment 343200 [details] [diff] [review]
without .get()
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/165c867e30ed
Crash Signature: [@ nsContentSubtreeIterator::Init]
http://hg.mozilla.org/mozilla-central/rev/4b28aee8d473
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.