Bugzilla@Mozilla – Bug 499844
"ASSERTION: not an element node: 'element'" with execCommand("outdent"), combining acute accent
Last modified: 2010-09-27 19:55:59 PDT
Summon comment box
Created attachment 384527 [details] testcase ###!!! ASSERTION: not an element node: 'element', file nsHTMLEditRules.cpp, line 8814 ###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file nsTArray.cpp, line 69
Created attachment 384528 [details] stack traces for the assertions
Security-sensitive because allocating negative-sized arrays scares me.
Daniel, can you either take a look at this one or find an owner?
CCing people who might be able to help with this editor security bug.
Ehsan might be a good person to work on this...
The second assertion happens because GetSkippedDistance assumes that the end offset is always larger than the start offset <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#2662>, which is not right in this particular case: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#5164>. I'm not sure why that happens, though. As for the first assertion, I don't see anything in this call site <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#4199> of RelativeChangeIndentationOfElementNode which ensure curNode is actually an element (it's an nsTextNode when this assertion occurs, for example.)
Created attachment 419973 [details] [review] Fix the first assertion This patch fixes the first assertion. The acute accent is not significant for the first assertion, but it is significant for the second one, and the second assertion still happens with this patch and the acute accent character.
Ehsan, Peterv you're still working on this, right?
So does the patch fix the second assertion or not? Comment 7 seems to imply it doesn't fix the assertion that this bug was filed on?
Comment on attachment 419973 [details] [review] Fix the first assertion >diff --git a/editor/libeditor/html/nsHTMLEditRules.cpp b/editor/libeditor/html/nsHTMLEditRules.cpp >--- a/editor/libeditor/html/nsHTMLEditRules.cpp >+++ b/editor/libeditor/html/nsHTMLEditRules.cpp >@@ -4191,17 +4191,20 @@ nsHTMLEditRules::WillOutdent(nsISelectio > } > curNode->GetLastChild(getter_AddRefs(child)); > } > // delete the now-empty list > res = mHTMLEditor->RemoveBlockContainer(curNode); > if (NS_FAILED(res)) return res; > } > else if (useCSS) { >- RelativeChangeIndentationOfElementNode(curNode, -1); >+ nsCOMPtr<nsIDOMElement> element = do_QueryInterface(curNode); >+ if (element) { Shouldn't we try to use curNode's parent if curNode is a texnode?
(In reply to comment #9) > So does the patch fix the second assertion or not? Comment 7 seems to imply it > doesn't fix the assertion that this bug was filed on? Comment 0 includes two assertions, and the attached patch fixes the first one.
Created attachment 426741 [details] [review] Fix the first assertion (In reply to comment #10) > (From update of attachment 419973 [details] [review]) > >diff --git a/editor/libeditor/html/nsHTMLEditRules.cpp b/editor/libeditor/html/nsHTMLEditRules.cpp > >--- a/editor/libeditor/html/nsHTMLEditRules.cpp > >+++ b/editor/libeditor/html/nsHTMLEditRules.cpp > >@@ -4191,17 +4191,20 @@ nsHTMLEditRules::WillOutdent(nsISelectio > > } > > curNode->GetLastChild(getter_AddRefs(child)); > > } > > // delete the now-empty list > > res = mHTMLEditor->RemoveBlockContainer(curNode); > > if (NS_FAILED(res)) return res; > > } > > else if (useCSS) { > >- RelativeChangeIndentationOfElementNode(curNode, -1); > >+ nsCOMPtr<nsIDOMElement> element = do_QueryInterface(curNode); > >+ if (element) { > > Shouldn't we try to use curNode's parent if curNode is a texnode? I think we should. This patch does that.
Created attachment 426747 [details] [review] Fix the first assertion Correct patch.
Created attachment 426750 [details] [review] Try to fix the second assertion The second assertion is mainly about nsTextFrame:GetPointFromOffset <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#5124>. I'm not really familiar with this code, but reading it, I think I can spot a problem. In this function, the properties object is first created using an iterator, and the iterator might get modified along the way <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#5168>, and later on the GetSkippedDistance call might end up comparing two different iterators. Something like this patch fixes the assertion, but I'm not sure if it's the correct solution. Asking review from roc to see what he thinks.
Comment on attachment 426747 [details] [review] Fix the first assertion >+ nsCOMPtr<nsIDOMElement> element = do_QueryInterface(curNode); Do the assignment in an else block after you checked for textNode.
Comment on attachment 426750 [details] [review] Try to fix the second assertion This isn't right. At this point "iter" is pointing to the character we want to find the offset to. We want to measure the distance from the first character to that character. Resetting the first character to iter is going to break that.
If you can explain the results of your debugging in more detail, we can probably work out what to do. But it might be simpler just to reassign the second assertion to me.
Created attachment 427219 [details] [review] Fix the assertion Updated patch based on comment 15.
(In reply to comment #17) > If you can explain the results of your debugging in more detail, we can > probably work out what to do. But it might be simpler just to reassign the > second assertion to me. I filed the second assertion as bug 546530, and I'm morphing this one to only cover the first one. I'll comment in that bug regarding comment 16.
(In reply to comment #17) > If you can explain the results of your debugging in more detail, we can > probably work out what to do. But it might be simpler just to reassign the > second assertion to me.
http://hg.mozilla.org/mozilla-central/rev/408816277180
Comment on attachment 427219 [details] [review] Fix the assertion a=beltzner for all branches
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7ba4a5b25791 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ebffcdc4a59d
Checking in editor/libeditor/html/nsHTMLEditRules.cpp; /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v <-- nsHTMLEditRules.cpp new revision: 1.345; previous revision: 1.344 done
Verified for 1.9.0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19pre) Gecko/2010031110 GranParadiso/3.0.19pre (.NET CLR 3.5.30729) using testcase. The assertion appears before the fix and no longer appears now. Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100311 Shiretoko/3.5.9pre (.NET CLR 3.5.30729). Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100310 Namoroka/3.6.2pre (.NET CLR 3.5.30729).
Test: http://hg.mozilla.org/mozilla-central/rev/82deac2893b0