499844 - "ASSERTION: not an element node: 'element'" with execCommand("outdent"), combining acute accent

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack traces for the assertions

Comment 2

[Jesse Ruderman]

Security-sensitive because allocating negative-sized arrays scares me.

Comment 3

[Brandon Sterne (:bsterne)]

Daniel, can you either take a look at this one or find an owner?

Comment 4

[Jesse Ruderman]

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)]

Ehsan might be a good person to work on this...

Comment 6

[(no longer active)]

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

[(no longer active)]

Attachment: 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][[e-mail]]]

Ehsan, Peterv you're still working on this, right?

Comment 9

[Peter Van der Beken [:peterv]]

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]]

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

[(no longer active)]

(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

[(no longer active)]

Attachment: 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

[(no longer active)]

Attachment: Fix the first assertion

Correct patch.

Comment 14

[(no longer active)]

Attachment: 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]]

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)]

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)]

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

[(no longer active)]

Attachment: Fix the assertion

Updated patch based on comment 15.

Comment 19

[(no longer active)]

(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

[(no longer active)]

(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 21

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/408816277180

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 427219 [details] [diff] [review]
Fix the assertion

a=beltzner for all branches

Comment 23

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7ba4a5b25791
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ebffcdc4a59d

Comment 24

[(no longer active)]

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 - ex-MoCo]]

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]

Test: http://hg.mozilla.org/mozilla-central/rev/82deac2893b0