Closed
Bug 761861
Opened 9 years ago
Closed 9 years ago
Crash with contentEditable, insertParagraph, forwardDelete
Categories
(Core :: DOM: Editor, defect)
Tracking
()
VERIFIED
FIXED
mozilla16
People
(Reporter: jruderman, Assigned: ayg)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, testcase)
Crash Data
Attachments
(3 files, 2 obsolete files)
417 bytes,
text/html
|
Details | |
12.77 KB,
text/plain
|
Details | |
2.34 KB,
patch
|
ehsan
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
Opt: bp-7cea9c29-e192-4e60-9a0e-069542120605
Comment 2•9 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
Assignee | ||
Comment 3•9 years ago
|
||
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.)
Assignee | ||
Comment 4•9 years ago
|
||
Okay, I can reproduce in an opt build only. Not sure why.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
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!
Attachment #630480 -
Attachment is obsolete: true
Attachment #630496 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•9 years ago
|
||
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?
Assignee | ||
Comment 7•9 years ago
|
||
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.
Updated•9 years ago
|
Keywords: regression
Version: Trunk → 15 Branch
Assignee | ||
Comment 8•9 years ago
|
||
[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.
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?
Updated•9 years ago
|
tracking-firefox15:
--- → +
Updated•9 years ago
|
Attachment #630503 -
Flags: review?(ehsan) → review+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4da1c7555ee
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Comment 10•9 years ago
|
||
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 12•9 years ago
|
||
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•9 years ago
|
||
Verified fixed on FF 16b1 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.7.4.
You need to log in
before you can comment on or make changes to this bug.
Description
•