336081 - Crash [@ nsHTMLEditRules::WillOutdent] involving designMode and removeformat

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

This is similar to bug 335995, but seems to involve different code and doesn't involve the <select> element.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Before crashing, a debug build spews:

###!!! ASSERTION: not an element node: 'element', file /Users/admin/trunk/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 8579

###!!! ASSERTION: null node passed to nsEditor::Tag(): 'aNode', file /Users/admin/trunk/mozilla/editor/libeditor/base/nsEditor.cpp, line 3977

WARNING: NS_ENSURE_TRUE(aNode) failed: file /Users/admin/trunk/mozilla/editor/libeditor/html/nsHTMLCSSUtils.cpp, line 1403

###!!! ASSERTION: null node passed to nsHTMLEditor::IsTableElement: 'node', file /Users/admin/trunk/mozilla/editor/libeditor/html/nsHTMLEditUtils.cpp, line 189

Comment 3

[Jesse Ruderman]

The testcase as-is doesn't work because it hits WRONG_DOCUMENT_ERR when trying to move nodes into the iframe, because of the change in bug 47903.  Once I fix that, it runs into NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR when trying to create the second range, thanks to the change in bug 336381.

--> Fixed or dup of bug 336381.

Comment 4

[Jesse Ruderman]

Attachment: testcase with importNode (wrong bug)

Comment 5

[Jesse Ruderman]

I even tried changing it to create r2 while its endpoints are still in the document.  Still no crash.

Comment 6

[Jesse Ruderman]

Oops, I mixed up this bug and bug 335995.  The last three comments (including the testcase) are all for bug 335995.

Comment 7

[Jesse Ruderman]

Attachment: testcase with importNode

Trunk Firefox still crashes.

Comment 8

[Jesse Ruderman]

This bug prevents me from testing execCommand well :(

Comment 9

[Jesse Ruderman]

Smaug/Martijn, can you fix this, or do you know who might be able to fix it?

Comment 10

[Martijn Wargers (dead)]

I bet Smaug can fix this ;)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

To fix the crash, it is enough to add simple |n &&| to the while(...)
where it is crashing, but that doesn't fix the assertions.

Comment 12

[Jesse Ruderman]

Thanks, that was enough to let me test execCommand a lot more than before.  Well, enough to find bug 372094, anyway :)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: to fix the crash

Could we take this simple null check for now?

Comment 14

[[e-mail]]

Comment on attachment 257969 [details] [diff] [review]
to fix the crash

Well, although this hides the crash, I think this is papering over the bug in GetNodeLocation for a node that has no parent (or indeed, document).

Comment 15

[Mike Connor [:mconnor]]

smaug, can you get SR and land this?  and file a followup on Neil's comment?

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 257969 [details] [diff] [review]
to fix the crash

Couldn't we just break (or continue?) immediately after GetNodeLocation? And we need a follow-up bug for the real fix, I wonder if we shouldn't make GetNodesFromSelection remove nodes that are not in a document.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I'll check in the null check fix so that testing execCommand is easier.
Bug 385895 filed.

Comment 18

[Jesse Ruderman]

Crashtest checked in.