Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing r?smaug
Even when editor receives eCompositionStart event, the active composition may have gone since web contents can listen to composition events before editor (so, web contents can commit the composition before "compositionstart" reaching focused editor).
Therefore, editor shouldn't crash as unexpected scenario. Instead, it should do nothing in such case.
Note that when editor receives 2nd or later "compositionupdate" or "text" event, it should not modify composition. However, currently, it does that. This patch does NOT fix it since it's really rare case. It should be fixed in another bug because this should be uplifted (crashing with some IMEs on Android is serious).
MozReview-Commit-ID: HIbMy4eFRMw
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -1995,36 +1995,41 @@ EditorBase::RestorePreservedSelection(Se
void
EditorBase::StopPreservingSelection()
{
mRangeUpdater.DropSelectionState(mSavedSel);
mSavedSel.MakeEmpty();
}
-void
+bool
EditorBase::EnsureComposition(WidgetCompositionEvent* aCompositionEvent)
{
if (mComposition) {
- return;
+ return true;
}
// The compositionstart event must cause creating new TextComposition
// instance at being dispatched by IMEStateManager.
mComposition = IMEStateManager::GetTextCompositionFor(aCompositionEvent);
if (!mComposition) {
- MOZ_CRASH("IMEStateManager doesn't return proper composition");
+ // However, TextComposition may be committed before the composition
+ // event comes here.
+ return false;
}
mComposition->StartHandlingComposition(this);
+ return true;
}
nsresult
EditorBase::BeginIMEComposition(WidgetCompositionEvent* aCompositionEvent)
{
MOZ_ASSERT(!mComposition, "There is composition already");
- EnsureComposition(aCompositionEvent);
+ if (!EnsureComposition(aCompositionEvent)) {
+ return NS_OK;
+ }
if (mPhonetic) {
mPhonetic->Truncate(0);
}
return NS_OK;
}
void
EditorBase::EndIMEComposition()
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -436,18 +436,24 @@ protected:
// or not.
return !IsPasswordEditor() && !IsReadonly() && !IsDisabled() &&
!ShouldSkipSpellCheck();
}
/**
* EnsureComposition() should be called by composition event handlers. This
* tries to get the composition for the event and set it to mComposition.
+ * However, this may fail because the composition may be committed before
+ * the event comes to the editor.
+ *
+ * @return true if there is a composition. Otherwise, for example,
+ * a composition event handler in web contents moved focus
+ * for committing the composition, returns false.
*/
- void EnsureComposition(WidgetCompositionEvent* aCompositionEvent);
+ bool EnsureComposition(WidgetCompositionEvent* aCompositionEvent);
nsresult GetSelection(SelectionType aSelectionType,
nsISelection** aSelection);
public:
/**
* All editor operations which alter the doc should be prefaced
* with a call to StartOperation, naming the action and direction.
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -839,17 +839,19 @@ TextEditor::UpdateIMEComposition(nsIDOME
MOZ_ASSERT(aDOMTextEvent, "aDOMTextEvent must not be nullptr");
WidgetCompositionEvent* compositionChangeEvent =
aDOMTextEvent->WidgetEventPtr()->AsCompositionEvent();
NS_ENSURE_TRUE(compositionChangeEvent, NS_ERROR_INVALID_ARG);
MOZ_ASSERT(compositionChangeEvent->mMessage == eCompositionChange,
"The internal event should be eCompositionChange");
- EnsureComposition(compositionChangeEvent);
+ if (!EnsureComposition(compositionChangeEvent)) {
+ return NS_OK;
+ }
nsCOMPtr<nsIPresShell> ps = GetPresShell();
NS_ENSURE_TRUE(ps, NS_ERROR_NOT_INITIALIZED);
RefPtr<Selection> selection = GetSelection();
NS_ENSURE_STATE(selection);
// NOTE: TextComposition should receive selection change notification before
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -194,16 +194,17 @@ subsuite = clipboard
skip-if = toolkit == 'android' # bug 1299578
[test_bug1153237.html]
[test_bug1154791.html]
skip-if = os == 'android'
[test_bug1162952.html]
[test_bug1181130-1.html]
[test_bug1181130-2.html]
[test_bug1186799.html]
+[test_bug1230473.html]
[test_bug1247483.html]
subsuite = clipboard
skip-if = toolkit == 'android'
[test_bug1248128.html]
[test_bug1250010.html]
[test_bug1257363.html]
[test_bug1248185.html]
[test_bug1258085.html]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug1230473.html
@@ -0,0 +1,124 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1230473
+-->
+<head>
+ <meta charset="utf-8">
+ <title>Test for Bug 1230473</title>
+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+ <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1230473">Mozilla Bug 1230473</a>
+<input id="input">
+<textarea id="textarea"></textarea>
+<div id="div" contenteditable></div>
+<script type="application/javascript">
+/** Test for Bug 1230473 **/
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(()=>{
+ function runTest(aEditor) {
+ function committer() {
+ aEditor.blur();
+ aEditor.focus();
+ }
+ function isNSEditableElement() {
+ return aEditor.tagName.toLowerCase() == "input" || aEditor.tagName.toLowerCase() == "textarea";
+ }
+ function value() {
+ return isNSEditableElement() ? aEditor.value : aEditor.textContent;
+ }
+ function isComposing() {
+ return isNSEditableElement() ? SpecialPowers.wrap(aEditor)
+ .QueryInterface(SpecialPowers.Ci.nsIDOMNSEditableElement)
+ .editor
+ .QueryInterface(SpecialPowers.Ci.nsIEditorIMESupport)
+ .composing :
+ SpecialPowers.wrap(window)
+ .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
+ .getInterface(SpecialPowers.Ci.nsIWebNavigation)
+ .QueryInterface(SpecialPowers.Ci.nsIDocShell)
+ .editor
+ .QueryInterface(SpecialPowers.Ci.nsIEditorIMESupport)
+ .composing;
+ }
+ function clear() {
+ if (isNSEditableElement()) {
+ aEditor.value = "";
+ } else {
+ aEditor.textContent = "";
+ }
+ }
+
+ clear();
+
+ // Committing at compositionstart
+ aEditor.focus();
+ aEditor.addEventListener("compositionstart", committer, true);
+ synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 1, length: 0 }});
+ aEditor.removeEventListener("compositionstart", committer, true);
+ ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionstart event handler");
+ is(value(), "", "composition in " + aEditor.id + " shouldn't insert any text since it's committed at compositionstart");
+ clear();
+
+ // Committing at first compositionupdate
+ aEditor.focus();
+ aEditor.addEventListener("compositionupdate", committer, true);
+ synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 1, length: 0 }});
+ aEditor.removeEventListener("compositionupdate", committer, true);
+ ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionupdate event handler");
+ is(value(), "", "composition in " + aEditor.id + " shouldn't have inserted any text since it's committed at first compositionupdate");
+ clear();
+
+ // Committing at first text (eCompositionChange)
+ aEditor.focus();
+ aEditor.addEventListener("text", committer, true);
+ synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 1, length: 0 }});
+ aEditor.removeEventListener("text", committer, true);
+ ok(!isComposing(), "composition in " + aEditor.id + " should be committed by text event handler");
+ is(value(), "", "composition in " + aEditor.id + " should have inserted any text since it's committed at first text");
+ clear();
+
+ // Committing at second compositionupdate
+ aEditor.focus();
+ synthesizeComposition({ type: "compositionstart" });
+ synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 1, length: 0 }});
+ ok(isComposing(), "composition should be in " + aEditor.id + " before dispatching second compositionupdate");
+ is(value(), "a", "composition in " + aEditor.id + " should be 'a' before dispatching second compositionupdate");
+ aEditor.addEventListener("compositionupdate", committer, true);
+ synthesizeCompositionChange({ composition: { string: "ab", clauses: [{length: 2, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 2, length: 0 }});
+ aEditor.removeEventListener("compositionupdate", committer, true);
+ ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionupdate event handler");
+ todo_is(value(), "a", "composition in " + aEditor.id + " shouldn't have been modified since it's committed at second compositionupdate");
+ clear();
+
+ // Committing at second text (eCompositionChange)
+ aEditor.focus();
+ synthesizeComposition({ type: "compositionstart" });
+ synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 1, length: 0 }});
+ ok(isComposing(), "composition should be in " + aEditor.id + " before dispatching second text");
+ is(value(), "a", "composition in " + aEditor.id + " should be 'a' before dispatching second text");
+ aEditor.addEventListener("text", committer, true);
+ synthesizeCompositionChange({ composition: { string: "ab", clauses: [{length: 2, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
+ caret: { start: 2, length: 0 }});
+ aEditor.removeEventListener("text", committer, true);
+ ok(!isComposing(), "composition in " + aEditor.id + " should be committed by text event handler");
+ todo_is(value(), "a", "composition in " + aEditor.id + " shouldn't have been modified since it's committed at second text");
+ clear();
+ }
+ runTest(document.getElementById("input"));
+ runTest(document.getElementById("textarea"));
+ runTest(document.getElementById("div"));
+ SimpleTest.finish();
+});
+</script>
+</body>
+</html>