Crash with contentEditable, insertParagraph, forwardDelete

VERIFIED FIXED in Firefox 15



7 years ago
6 years ago


(Reporter: jruderman, Assigned: ayg)


(Blocks: 1 bug, {crash, regression, testcase})

15 Branch
crash, regression, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ verified, firefox16+ verified)


(crash signature)


(3 attachments, 2 obsolete attachments)



7 years ago
Created attachment 630374 [details]
testcase (crashes Firefox when loaded)

Comment 2

7 years ago
On Windows: bp-8ef6b204-d35a-4653-8332-ddd4e2120606.
Crash Signature: [@ nsHTMLEditor::DeleteSelectionImpl] → [@ nsHTMLEditor::DeleteSelectionImpl] [@ nsINode::GetParent()]
OS: Mac OS X → All
Hardware: x86_64 → All
Created attachment 630480 [details] [diff] [review]
Possible patch, untested

I can see what the problem should be, and this patch should fix it.  But I can't reproduce the crash on a debug build I compile myself at r95861.  I only see it in a nightly.  Can anyone confirm that the problem still exists on latest trunk, or failing that, explain where it does exist?  (We need a fix to backport to aurora, at the very least.)
Okay, I can reproduce in an opt build only.  Not sure why.
Assignee: nobody → ayg
Created attachment 630496 [details] [diff] [review]
Patch v2, with test


This also demonstrated something really weird -- insertParagraph doesn't ignore the last argument.  Something is pretty badly wrong with my implementation if that's the case!
Attachment #630480 - Attachment is obsolete: true
Attachment #630496 - Flags: review?(ehsan)
Comment on attachment 630496 [details] [diff] [review]
Patch v2, with test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 590640, patch part 5.
User impact if declined: Easily-triggered crash, although AFAICT the conditions are relatively unlikely to occur in non-malicious pages (editable root element with non-block children).
Testing completed (on m-c, etc.): None yet; requesting approval in parallel with review request.  See try run link in previous comment.
Risk to taking this patch (and alternatives if risky): Minimal -- simple null check.  Backing out bug 590640 patch 5 would also fix the issue, but would involve much larger changes.  That patch changes over 200 LOC by itself (not counting tests), and backing it out would conflict with a lot of subsequent patches.
String or UUID changes made by this patch: None.
Attachment #630496 - Flags: approval-mozilla-aurora?
This is really caused by bug 590640, not bug 748307.  Bug 590640 part 5 added the problematic code.  I'm pretty sure it's possible to trigger this solely with commands that existed before bug 748307 landed -- construct the DOM yourself instead of using insertParagraph, and use delete instead of forwardDelete.
Blocks: 590640
No longer blocks: 748307


7 years ago
Keywords: regression
Version: Trunk → 15 Branch
Created attachment 630503 [details] [diff] [review]
Patch v3, with better test

[Approval Request Comment] See comment 6.  This is the same patch with a different test.

The new test doesn't rely on the fact that insertParagraph incorrectly works the same as insertText when execCommand() is given a third parameter.  It uses only execCommand("forwarddelete").  That's not needed either; you can do it without execCommand() at all.  Just load this URL, put the cursor before the underscore, and hit the delete key to delete the next character:

data:text/html,<!doctype html>
function boom() {
  var r = document.documentElement;
  while (r.firstChild) {

  document.documentElement.contentEditable = "true";
<body onload="boom()">

Thus bug 748307 isn't needed for the crash at all (but it makes testing easier).

New try run:

It's worth wondering why forward and backward delete are going down such different codepaths here.  I'd have expected both to trigger the bug.
Attachment #630496 - Attachment is obsolete: true
Attachment #630496 - Flags: review?(ehsan)
Attachment #630496 - Flags: approval-mozilla-aurora?
Attachment #630503 - Flags: review?(ehsan)
Attachment #630503 - Flags: approval-mozilla-aurora?


7 years ago
tracking-firefox15: --- → +


7 years ago
Attachment #630503 - Flags: review?(ehsan) → review+

Comment 9

7 years ago
Last Resolved: 7 years ago
Resolution: --- → FIXED


7 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Comment on attachment 630503 [details] [diff] [review]
Patch v3, with better test

[Triage Comment]
Simple null check, approved for Aurora 15.
Attachment #630503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

7 years ago
status-firefox15: --- → fixed
status-firefox16: --- → fixed
tracking-firefox16: --- → +
No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.
status-firefox15: fixed → verified
Verified fixed on FF 16b1 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.7.4.
status-firefox16: fixed → verified
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.