Closed
Bug 499844
Opened 16 years ago
Closed 15 years ago
"ASSERTION: not an element node: 'element'" with execCommand("outdent"), combining acute accent
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a2
People
(Reporter: jruderman, Assigned: ehsan.akhgari)
References
Details
(5 keywords, Whiteboard: [sg:critical?])
Attachments
(3 files, 4 obsolete files)
|
252 bytes,
text/html
|
Details | |
|
18.92 KB,
text/plain
|
Details | |
|
1.57 KB,
patch
|
beltzner
:
approval1.9.2.2+
beltzner
:
approval1.9.1.9+
beltzner
:
approval1.9.0.19+
|
Details | Diff | Splinter Review |
###!!! 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•16 years ago
|
||
| Reporter | ||
Comment 2•16 years ago
|
||
Security-sensitive because allocating negative-sized arrays scares me.
| Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?]
Comment 3•16 years ago
|
||
Daniel, can you either take a look at this one or find an owner?
Assignee: nobody → daniel
| Reporter | ||
Updated•15 years ago
|
Assignee: daniel → nobody
| Reporter | ||
Comment 4•15 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•15 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•15 years ago
|
||
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•15 years ago
|
||
Ehsan, Peterv you're still working on this, right?
Comment 9•15 years ago
|
||
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•15 years ago
|
||
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•15 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•15 years ago
|
||
(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•15 years ago
|
||
Correct patch.
Attachment #426741 -
Attachment is obsolete: true
Attachment #426747 -
Flags: review?(peterv)
Attachment #426741 -
Flags: review?(peterv)
| Assignee | ||
Comment 14•15 years ago
|
||
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•15 years ago
|
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Comment 15•15 years ago
|
||
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•15 years ago
|
||
Updated patch based on comment 15.
Attachment #426747 -
Attachment is obsolete: true
Attachment #426750 -
Attachment is obsolete: true
| Assignee | ||
Comment 19•15 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•15 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•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][ready to land] → [sg:critical?]
Target Milestone: --- → mozilla1.9.3a2
| Assignee | ||
Updated•15 years ago
|
Attachment #427219 -
Flags: approval1.9.2.2?
Attachment #427219 -
Flags: approval1.9.1.9?
Attachment #427219 -
Flags: approval1.9.0.19?
Comment 22•15 years ago
|
||
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•15 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•15 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
Comment 25•15 years ago
|
||
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).
OS: Mac OS X → All
Updated•15 years ago
|
Group: core-security
| Reporter | ||
Comment 26•15 years ago
|
||
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•