Closed Bug 336091 Opened 14 years ago Closed 9 years ago

"ASSERTION: bad action nesting!" using execCommand('justifyleft') with no selection

Categories

(Core :: DOM: Editor, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: ehsan)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 2 obsolete files)

###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file /Users/admin/trunk/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 386

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: bad-nesting.xhtml :: anonymous :: line 17"  data: no]
Attached file testcase
Blocks: 336383
If I try to type into the iframe after this bug has been triggered, I see the assertion again and my text does not appear.
Still happens on trunk.
Assignee: mozeditor → nobody
QA Contact: editor
The problem is that some code assumes that the selection has at least one range in it, which is not the case in this testcase.

This patch adds code to functions that are failing, to check for selection without any ranges. We still return failure when there's no selection, so our behaviour still matches FF2, and we still throw a JS exception when the selection doesn't contain any ranges. There's no assertions failing now, and we're properly incrementing/decrementing the nsHTMLEditRules::mActionNesting field.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #293452 - Flags: review?(peterv)
Comment on attachment 293452 [details] [diff] [review]
Patch v1 - handle empty selection

>Index: editor/libeditor/html/nsHTMLEditRules.cpp
>===================================================================

>+    

Trailing whitespace.

>+    // Get number of ranges in selection.
>+    PRInt32 rangeCount=0;

Spaces around =

>+    nsresult rv = selection->GetRangeCount(&rangeCount);
>+    NS_ENSURE_SUCCESS(rv, rv);

Use the existing res instead of rv.

>+    if (rangeCount) {

    if (rangeCount > 0)

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>===================================================================

>+  

Trailing whitespace.

>+  PRInt32 rangeCount=0;

Spaces around =.

>+  nsresult rv = selection->GetRangeCount(&rangeCount);
>+  NS_ENSURE_SUCCESS(rv, rv);

res

>+  if (!rangeCount) {

rangeCount == 0

Could you check if IE throws an exception in this case? Might be good to try to be compatible.
Attachment #293452 - Flags: superreview+
Attachment #293452 - Flags: review?(peterv)
Attachment #293452 - Flags: review+
(In reply to comment #5)
> Could you check if IE throws an exception in this case? Might be good to try to
> be compatible.

I don't see IE7 throwing an exception when I put the following test case into it:

<html xmlns="http://www.w3.org/1999/xhtml">
<head></head>

<script>
function init() {
  setTimeout(function() {
    var editDoc = frames['editFrame'].document;
    editDoc.designMode='On';
    editDoc.selection.empty();
    editDoc.execCommand('justifyleft', false, null);
  }, 500);
}
</script>

<body onload="init();">
<iframe id="editFrame" style="width:200px;height:200px;"></iframe>
</body>

</html>

I'm pretty sure that's equivalent to the Firefox version. So to be compatible, we'd need to change the justify command so that it doesn't throw when it's passed an empty selection, just return. Do we want to do that?

There's another way you can get this assertion to fail - if you load the document, then reload and start bashing keys, the assertion fires. This is because the key events for your key bashing are being delivered to the editable iframe, but the iframe has no selection (as it's not been focused yet) and its failing. The problem is that the iframe is being sent key events even though it's not focused (in this other case).
Attached patch v3 - wip (obsolete) — Splinter Review
This is the patch peterv and I developed - it almost fixes things. It stops the assertion failures. I need to look further to figure out what's causing the JS error.
Attachment #293452 - Attachment is obsolete: true
Depends on: 496337
Blocks: 481097
WFM
Status: ASSIGNED → NEW
(In reply to comment #9)
> WFM

Did you mean to resolve this as WFM?
I didn't want to just mark it as WFM becaues it has a WIP patch.
The first hunk of the WIP patch is the only thing which is still needed here, I think (although the original problem of the bug has been fixed a while ago).
Assignee: chris → ehsan
Attachment #320300 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #511095 - Flags: review?(roc)
Attachment #511095 - Flags: approval2.0?
Attachment #511095 - Flags: review?(roc)
Attachment #511095 - Flags: review+
Attachment #511095 - Flags: approval2.0?
Attachment #511095 - Flags: approval2.0+
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/3ce0b8db2ebb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.