Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 08 Nov 2016 00:36:32 +0900
changeset 348328 6184510d1b737b0a9cde17c5144af2891affa0d5
parent 348327 53049dd09053d96543932c00a2bdcee7bc3118b7
child 348329 4777ed22d16ddef936a98097d709d6537be6ef2d
push id10298
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:33:03 +0000
treeherdermozilla-aurora@7e29173b1641 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1230473
milestone52.0a1
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
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/TextEditor.cpp
editor/libeditor/tests/mochitest.ini
editor/libeditor/tests/test_bug1230473.html
--- 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>