Last Comment Bug 499844 - "ASSERTION: not an element node: 'element'" with execCommand("outdent"), combining acute accent
: "ASSERTION: not an element node: 'element'" with execCommand("outdent"), comb...
Status: RESOLVED FIXED
[sg:critical?]
: assertion, testcase, verified1.9.0.19, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla1.9.3a2
Assigned To: :Ehsan Akhgari
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 336383 textfuzzer
  Show dependency treegraph
 
Reported: 2009-06-22 17:18 PDT by Jesse Ruderman
Modified: 2010-09-27 19:55 PDT (History)
13 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
.9-fixed


Attachments
testcase (252 bytes, text/html)
2009-06-22 17:18 PDT, Jesse Ruderman
no flags Details
stack traces for the assertions (18.92 KB, text/plain)
2009-06-22 17:19 PDT, Jesse Ruderman
no flags Details
Fix the first assertion (1.00 KB, patch)
2010-01-04 14:15 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Fix the first assertion (1.22 KB, patch)
2010-02-12 13:54 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Fix the first assertion (1.53 KB, patch)
2010-02-12 14:27 PST, :Ehsan Akhgari
peterv: review+
Details | Diff | Splinter Review
Try to fix the second assertion (1.46 KB, patch)
2010-02-12 14:41 PST, :Ehsan Akhgari
roc: review-
Details | Diff | Splinter Review
Fix the assertion (1.57 KB, patch)
2010-02-16 14:55 PST, :Ehsan Akhgari
mbeltzner: approval1.9.2.2+
mbeltzner: approval1.9.1.9+
mbeltzner: approval1.9.0.19+
Details | Diff | Splinter Review

Description Jesse Ruderman 2009-06-22 17:18:55 PDT
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
Comment 1 Jesse Ruderman 2009-06-22 17:19:30 PDT
Created attachment 384528 [details]
stack traces for the assertions
Comment 2 Jesse Ruderman 2009-06-22 17:26:41 PDT
Security-sensitive because allocating negative-sized arrays scares me.
Comment 3 Brandon Sterne (:bsterne) 2009-10-08 16:48:08 PDT
Daniel, can you either take a look at this one or find an owner?
Comment 4 Jesse Ruderman 2009-12-29 12:25:33 PST
CCing people who might be able to help with this editor security bug.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-30 13:29:30 PST
Ehsan might be a good person to work on this...
Comment 6 :Ehsan Akhgari 2010-01-04 12:46:19 PST
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.)
Comment 7 :Ehsan Akhgari 2010-01-04 14:15:12 PST
Created attachment 419973 [details] [diff] [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.
Comment 8 Olli Pettay [:smaug] 2010-02-07 12:12:36 PST
Ehsan, Peterv you're still working on this, right?
Comment 9 Peter Van der Beken [:peterv] 2010-02-11 03:26:53 PST
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 10 Peter Van der Beken [:peterv] 2010-02-11 04:38:00 PST
Comment on attachment 419973 [details] [diff] [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?
Comment 11 :Ehsan Akhgari 2010-02-12 13:36:42 PST
(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.
Comment 12 :Ehsan Akhgari 2010-02-12 13:54:46 PST
Created attachment 426741 [details] [diff] [review]
Fix the first assertion

(In reply to comment #10)
> (From update of attachment 419973 [details] [diff] [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.
Comment 13 :Ehsan Akhgari 2010-02-12 14:27:38 PST
Created attachment 426747 [details] [diff] [review]
Fix the first assertion

Correct patch.
Comment 14 :Ehsan Akhgari 2010-02-12 14:41:33 PST
Created attachment 426750 [details] [diff] [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 15 Peter Van der Beken [:peterv] 2010-02-13 02:52:02 PST
Comment on attachment 426747 [details] [diff] [review]
Fix the first assertion

>+          nsCOMPtr<nsIDOMElement> element = do_QueryInterface(curNode);

Do the assignment in an else block after you checked for textNode.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-02-15 00:10:43 PST
Comment on attachment 426750 [details] [diff] [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.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-02-15 00:16:11 PST
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.
Comment 18 :Ehsan Akhgari 2010-02-16 14:55:40 PST
Created attachment 427219 [details] [diff] [review]
Fix the assertion

Updated patch based on comment 15.
Comment 19 :Ehsan Akhgari 2010-02-16 15:09:58 PST
(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.
Comment 20 :Ehsan Akhgari 2010-02-16 15:16:23 PST
(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.
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2010-02-26 13:10:26 PST
Comment on attachment 427219 [details] [diff] [review]
Fix the assertion

a=beltzner for all branches
Comment 24 :Ehsan Akhgari 2010-03-01 13:06:20 PST
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
Comment 25 Al Billings [:abillings] 2010-03-11 17:44:25 PST
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).
Comment 26 Jesse Ruderman 2010-09-27 19:55:59 PDT
Test: http://hg.mozilla.org/mozilla-central/rev/82deac2893b0

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