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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: celelibi, Assigned: smaug)
References
()
Details
(Keywords: crash, testcase, verified1.8)
Crash Data
Attachments
(5 files, 2 obsolete files)
478 bytes,
text/html
|
Details | |
1.32 KB,
patch
|
brendan
:
review+
brendan
:
superreview+
brendan
:
approval1.8b4+
|
Details | Diff | Splinter Review |
81.15 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
83.49 KB,
patch
|
sicking
:
superreview+
|
Details | Diff | Splinter Review |
83.48 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•19 years ago
|
||
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•19 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•19 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•19 years ago
|
Flags: blocking1.8b4?
Comment 3•19 years ago
|
||
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•19 years ago
|
Updated•19 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 5•19 years ago
|
||
when did this crash start happening? I don't see it in the topcrash reports. is this severe?
Updated•19 years ago
|
Assignee: traversal-range → mrbkap
Flags: blocking1.8b4? → blocking1.8b4-
Comment 6•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
We'll take crash defense patches that don't add risk. /be
Flags: blocking1.8b4- → blocking1.8b4+
Comment 10•19 years ago
|
||
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+
Comment 11•19 years ago
|
||
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]
Comment 12•19 years ago
|
||
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 14•18 years ago
|
||
Reference http://wiki.mozilla.org/DOM:ContentIterator_Issues .
Flags: blocking1.9-
Assignee | ||
Comment 15•16 years ago
|
||
With bug 332148 gives one point in ACID3. But the patch is really just WIP.
Comment 16•16 years ago
|
||
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•16 years ago
|
||
Yes, I'll probably split the patch.
Updated•16 years ago
|
Flags: wanted1.9.1+
Assignee | ||
Comment 18•16 years ago
|
||
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•16 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)
Comment 20•16 years ago
|
||
Smaug, does that patch also make things work when the range start and end parents are both the Document?
Updated•16 years ago
|
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.
Comment 22•16 years ago
|
||
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•16 years ago
|
||
NS_RELEASE(mChild), fixes the leak, at least here. Gives 2 points in ACID3.
Attachment #338638 -
Flags: review?(peterv)
Assignee | ||
Updated•16 years ago
|
Attachment #332418 -
Attachment is obsolete: true
Attachment #332418 -
Flags: review?(peterv)
Attachment #332418 -
Flags: review?(jst)
Assignee | ||
Comment 24•16 years ago
|
||
(In reply to comment #23) > Gives 2 points in ACID3. Tests 7 and 10
Assignee | ||
Comment 25•16 years ago
|
||
And now 3 points, 7,10 and 11.
Assignee | ||
Comment 26•16 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•16 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•16 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 ?
Comment 30•16 years ago
|
||
You should be applying the patch from the root of the tree (not one dir above), with -p1, no?
Comment 31•16 years ago
|
||
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•16 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•16 years ago
|
||
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•16 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•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 37•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/165c867e30ed
Updated•13 years ago
|
Crash Signature: [@ nsContentSubtreeIterator::Init]
Comment 38•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4b28aee8d473
Updated•11 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•