The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Ehsan)

Tracking

(Blocks: 2 bugs, 5 keywords)

Trunk
mozilla1.9.3a2
x86
All
assertion, testcase, verified1.9.0.19, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .2-fixed, status1.9.1 .9-fixed)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

8 years ago
Created attachment 384528 [details]
stack traces for the assertions
(Reporter)

Comment 2

8 years ago
Security-sensitive because allocating negative-sized arrays scares me.
(Reporter)

Updated

8 years ago
Whiteboard: [sg:critical?]
Daniel, can you either take a look at this one or find an owner?
Assignee: nobody → daniel
(Reporter)

Updated

7 years ago
Assignee: daniel → nobody
(Reporter)

Comment 4

7 years ago
CCing people who might be able to help with this editor security bug.
Ehsan might be a good person to work on this...
(Assignee)

Comment 6

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

Comment 7

7 years ago
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.
Attachment #419973 - Flags: review?(peterv)

Comment 8

7 years ago
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] [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?
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

7 years ago
Created attachment 426747 [details] [diff] [review]
Fix the first assertion

Correct patch.
Attachment #426741 - Attachment is obsolete: true
Attachment #426747 - Flags: review?(peterv)
Attachment #426741 - Flags: review?(peterv)
(Assignee)

Comment 14

7 years ago
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.
Attachment #426750 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
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.
Attachment #426747 - Flags: review?(peterv) → review+
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.
Attachment #426750 - Flags: review?(roc) → review-
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.
(Assignee)

Comment 18

7 years ago
Created attachment 427219 [details] [diff] [review]
Fix the assertion

Updated patch based on comment 15.
Attachment #426747 - Attachment is obsolete: true
Attachment #426750 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
(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.
Summary: "ASSERTION: Attempting to allocate excessively large array" with execCommand("outdent"), combining acute accent → "ASSERTION: not an element node: 'element'" with execCommand("outdent"), combining acute accent
(Assignee)

Comment 20

7 years ago
(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.
Whiteboard: [sg:critical?] → [sg:critical?][ready to land]
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/mozilla-central/rev/408816277180
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][ready to land] → [sg:critical?]
Target Milestone: --- → mozilla1.9.3a2
(Assignee)

Updated

7 years ago
Attachment #427219 - Flags: approval1.9.2.2?
Attachment #427219 - Flags: approval1.9.1.9?
Attachment #427219 - Flags: approval1.9.0.19?
Comment on attachment 427219 [details] [diff] [review]
Fix the assertion

a=beltzner for all branches
Attachment #427219 - Flags: approval1.9.2.2?
Attachment #427219 - Flags: approval1.9.2.2+
Attachment #427219 - Flags: approval1.9.1.9?
Attachment #427219 - Flags: approval1.9.1.9+
Attachment #427219 - Flags: approval1.9.0.19?
Attachment #427219 - Flags: approval1.9.0.19+
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7ba4a5b25791
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ebffcdc4a59d
status1.9.1: --- → .9-fixed
status1.9.2: --- → .2-fixed
(Assignee)

Comment 24

7 years ago
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
Keywords: fixed1.9.0.19
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).
Keywords: fixed1.9.0.19 → verified1.9.0.19, verified1.9.1, verified1.9.2
OS: Mac OS X → All
Group: core-security
(Reporter)

Comment 26

7 years ago
Test: http://hg.mozilla.org/mozilla-central/rev/82deac2893b0
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.