Last Comment Bug 761861 - Crash with contentEditable, insertParagraph, forwardDelete
: Crash with contentEditable, insertParagraph, forwardDelete
Status: VERIFIED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 15 Branch
: All All
: -- critical (vote)
: mozilla16
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
: Paul Silaghi, QA [:pauly]
:
Mentors:
Depends on:
Blocks: 336383 590640
  Show dependency treegraph
 
Reported: 2012-06-05 17:03 PDT by Jesse Ruderman
Modified: 2012-08-30 06:08 PDT (History)
5 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
testcase (crashes Firefox when loaded) (417 bytes, text/html)
2012-06-05 17:03 PDT, Jesse Ruderman
no flags Details
stack trace (12.77 KB, text/plain)
2012-06-05 17:03 PDT, Jesse Ruderman
no flags Details
Possible patch, untested (1.26 KB, patch)
2012-06-06 02:05 PDT, Aryeh Gregor (:ayg) (away until October 25)
no flags Details | Diff | Splinter Review
Patch v2, with test (2.30 KB, patch)
2012-06-06 03:52 PDT, Aryeh Gregor (:ayg) (away until October 25)
no flags Details | Diff | Splinter Review
Patch v3, with better test (2.34 KB, patch)
2012-06-06 04:22 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-05 17:03:27 PDT
Created attachment 630374 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2012-06-05 17:03:50 PDT
Created attachment 630375 [details]
stack trace

Opt: bp-7cea9c29-e192-4e60-9a0e-069542120605
Comment 2 Scoobidiver (away) 2012-06-05 23:25:00 PDT
On Windows: bp-8ef6b204-d35a-4653-8332-ddd4e2120606.
Comment 3 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 02:05:08 PDT
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.)
Comment 4 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 03:07:31 PDT
Okay, I can reproduce in an opt build only.  Not sure why.
Comment 5 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 03:52:16 PDT
Created attachment 630496 [details] [diff] [review]
Patch v2, with test

Try: https://tbpl.mozilla.org/?tree=Try&rev=d5d4c9ef038a

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!
Comment 6 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 04:00:13 PDT
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.
Comment 7 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 04:01:57 PDT
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.
Comment 8 Aryeh Gregor (:ayg) (away until October 25) 2012-06-06 04:22:51 PDT
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>
<script>
function boom() {
  var r = document.documentElement;
  while (r.firstChild) {
    r.removeChild(r.firstChild);
  }

  document.documentElement.contentEditable = "true";
  document.documentElement.appendChild(document.createElement("span"));
  document.documentElement.firstChild.appendChild(document.createTextNode("_"));
}
</script>
<body onload="boom()">

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

New try run: https://tbpl.mozilla.org/?tree=Try&rev=1427204ad017

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.
Comment 10 Alex Keybl [:akeybl] 2012-06-11 12:56:47 PDT
Comment on attachment 630503 [details] [diff] [review]
Patch v3, with better test

[Triage Comment]
Simple null check, approved for Aurora 15.
Comment 12 Paul Silaghi, QA [:pauly] 2012-08-06 07:38:14 PDT
No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.
Comment 13 Paul Silaghi, QA [:pauly] 2012-08-30 06:08:15 PDT
Verified fixed on FF 16b1 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.7.4.

Note You need to log in before you can comment on or make changes to this bug.