Closed
Bug 434766
Opened 17 years ago
Closed 16 years ago
Crash [@ nsHTMLEditRules::WillDoAction] with moving contenteditable body in head, and undo execCommand
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
| Tracking | Status | |
|---|---|---|
| status1.9.1 | --- | wontfix |
People
(Reporter: martijn.martijn, Assigned: mrbkap)
References
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:dos] null-deref)
Crash Data
Attachments
(2 files, 1 obsolete file)
|
655 bytes,
text/html
|
Details | |
|
1.89 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
See testcase, which crashes current trunk build after 100ms.
This regressed between 2007-10-25 and 2007-10-26:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-25+04&maxdate=2007-10-26+09&cvsroot=%2Fcvsroot
I think a regression from bug 395340.
The iframe content is this:
<html><head>
</head>
<body contenteditable="true">
<script>
document.getElementsByTagName('head')[0].appendChild(document.body);
window.frameElement.style.display = 'none';
function doe(){
document.execCommand('undo', false, '');
}
setTimeout(doe,100);
</script>
</body>
</html>
http://crash-stats.mozilla.com/report/index/ed5635e6-2686-11dd-b656-001a4bd46e84?p=1
0 xul.dll nsHTMLEditRules::WillDoAction mozilla/editor/libeditor/html/nsHTMLEditRules.cpp:605
1 xul.dll nsPlaintextEditor::Undo mozilla/editor/libeditor/text/nsPlaintextEditor.cpp:1104
2 xul.dll nsUndoCommand::DoCommand mozilla/editor/libeditor/base/nsEditorCommands.cpp:90
3 xul.dll nsControllerCommandTable::DoCommand mozilla/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191
4 xul.dll nsBaseCommandController::DoCommand mozilla/embedding/components/commandhandler/src/nsBaseCommandController.cpp:169
5 xul.dll nsCommandManager::DoCommand mozilla/embedding/components/commandhandler/src/nsCommandManager.cpp:272
6 xul.dll nsHTMLDocument::ExecCommand mozilla/content/html/document/src/nsHTMLDocument.cpp:4489
7 js3250.dll js_GetClassObject mozilla/js/src/jsobj.c:2653
Flags: blocking1.9.1? → wanted1.9.1+
|
||
Comment 3•16 years ago
|
||
This testcase crashes due to a null selection:
rv = aSelection->GetRange(0, getter_AddRefs(domRange));
Group: core-security
Whiteboard: [sg:nse dos] null-deref
|
||
Updated•16 years ago
|
Flags: wanted1.8.1.x-
|
|
||
Comment 4•16 years ago
|
||
(I tested this with a mozilla-central nightly build on WinXP SP3)
0:000> !exploitable
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at xul!nsHTMLEditRules::WillDoAction+0x61 (Hash=0x7a647042.0x156a487c)
The data from the faulting address is later used as the target for a branch.
|
Assignee | |
Comment 5•16 years ago
|
||
I don't know if it's right to bail before the nsAutoRules, but this is the easiest fix for this bug that doesn't introduce any new assertions (which nsAutoRules does does if you don't call DidDoAction).
I failed at making a crashtest for this bug and don't have time to look into it now.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #372514 -
Flags: superreview?(peterv)
Attachment #372514 -
Flags: review?(peterv)
|
||
Comment 6•16 years ago
|
||
Comment on attachment 372514 [details] [diff] [review]
Easiest fix
I think we should actually make WillDoAction deal with a null selection.
Attachment #372514 -
Flags: superreview?(peterv)
Attachment #372514 -
Flags: superreview-
Attachment #372514 -
Flags: review?(peterv)
Attachment #372514 -
Flags: review-
|
|
||
Updated•16 years ago
|
Whiteboard: [sg:nse dos] null-deref → [sg:dos] null-deref
|
|
Assignee | |
Comment 7•16 years ago
|
||
So, this addresses peterv's comment, but introduces a new assertion:
###!!! ASSERTION: bad action nesting!: 'mActionNesting>0'
As far as I can tell, this is because there's a dependency on having a non-null selection further down the call stack (under nsHTMLEditRules::BeforeEdit) which ends up bailing after doing a bunch of work, but before incrementing mActionNesting. So later, when we unwind the stack, we hit the assertion. I really have no clue what the right thing to do here is.
Attachment #372514 -
Attachment is obsolete: true
Attachment #376282 -
Flags: superreview?(peterv)
Attachment #376282 -
Flags: review?(peterv)
|
|
||
Updated•16 years ago
|
Attachment #376282 -
Flags: superreview?(peterv)
Attachment #376282 -
Flags: superreview+
Attachment #376282 -
Flags: review?(peterv)
Attachment #376282 -
Flags: review+
|
|
||
Comment 8•16 years ago
|
||
Comment on attachment 376282 [details] [diff] [review]
Updated
I think you should just make the two BeforeEdit implementations increment mActionNesting at the top, and change the checks for !mActionNesting to checks for mActionNesting == 1. AFAICT we'll never end up with bad nesting since we don't even call BeforeEdit/AfterEdit for nested actions (nsAutoRules protects against that). See also bug 496337 that I filed for cleaning up mActionNesting.
|
|
Assignee | |
Comment 9•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b5bdefc2cac7
I left the mActionNesting stuff for bug 496337.
Shouldn't this be marked FIXED?
|
|
Assignee | |
Comment 11•16 years ago
|
||
Oops, yes.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: blocking1.9.2?
|
|
||
Comment 13•16 years ago
|
||
We don't need to fix this non-exploitable crash on old branches (unless it becomes a topcrash) -- 3.6 is coming soon enough.
status1.9.1:
--- → wontfix
Flags: wanted1.9.0.x-
|
||
Updated•14 years ago
|
Crash Signature: [@ nsHTMLEditRules::WillDoAction]
You need to log in
before you can comment on or make changes to this bug.
Description
•