Bug 1368888 - Don't get previous value twice in input.value setter. r=smaug
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Wed, 14 Jun 2017 18:21:01 +0900
changeset 364550 4b9cc1cd08ea57cc83ee49d996794846537c4ca7
parent 364549 d9e4073abd4cb2166768abfa669190ac7a0b8f53
child 364551 21c6a3e5c9914b53d757a38da93e9147550912e9
push id32047
push userarchaeopteryx@coole-files.de
push dateSun, 18 Jun 2017 18:28:16 +0000
treeherdermozilla-central@ae20db43e815 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1368888
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368888 - Don't get previous value twice in input.value setter. r=smaug We get previous input.value twice in HTMLInputElement::SetValue and nsTextEditorState::SetValue when setting input.value. Since nsTextEditorState::GetValue uses DocumentEncoder, it is expensive. So we should use old value as parameter of nsTextEditorState::SetValue if possible. MozReview-Commit-ID: A1UPfETTVCn
dom/html/HTMLInputElement.cpp
dom/html/HTMLInputElement.h
dom/html/nsTextEditorState.cpp
dom/html/nsTextEditorState.h
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -1868,18 +1868,22 @@ HTMLInputElement::SetValue(const nsAStri
       // event, we should keep it that way. Otherwise, we should make sure the
       // element will not fire any event because of the script interaction.
       //
       // NOTE: this is currently quite expensive work (too much string
       // manipulation). We should probably optimize that.
       nsAutoString currentValue;
       GetValue(currentValue, aCallerType);
 
+      // Some types sanitize value, so GetValue doesn't return pure
+      // previous value correctly.
       nsresult rv =
         SetValueInternal(aValue,
+          (IsExperimentalMobileType(mType) || IsDateTimeInputType(mType)) ?
+            nullptr : &currentValue,
           nsTextEditorState::eSetValue_ByContent |
           nsTextEditorState::eSetValue_Notify |
           nsTextEditorState::eSetValue_MoveCursorToEndIfValueChanged);
       if (NS_FAILED(rv)) {
         aRv.Throw(rv);
         return;
       }
 
@@ -3027,17 +3031,19 @@ HTMLInputElement::UpdateFileList()
       if (array[i].IsFile()) {
         mFileData->mFileList->Append(array[i].GetAsFile());
       }
     }
   }
 }
 
 nsresult
-HTMLInputElement::SetValueInternal(const nsAString& aValue, uint32_t aFlags)
+HTMLInputElement::SetValueInternal(const nsAString& aValue,
+                                   const nsAString* aOldValue,
+                                   uint32_t aFlags)
 {
   NS_PRECONDITION(GetValueMode() != VALUE_MODE_FILENAME,
                   "Don't call SetValueInternal for file inputs");
 
   switch (GetValueMode()) {
     case VALUE_MODE_VALUE:
     {
       // At the moment, only single line text control have to sanitize their value
@@ -3051,17 +3057,17 @@ HTMLInputElement::SetValueInternal(const
       // else DoneCreatingElement calls us again once mDoneCreating is true
 
       bool setValueChanged = !!(aFlags & nsTextEditorState::eSetValue_Notify);
       if (setValueChanged) {
         SetValueChanged(true);
       }
 
       if (IsSingleLineTextControl(false)) {
-        if (!mInputData.mState->SetValue(value, aFlags)) {
+        if (!mInputData.mState->SetValue(value, aOldValue, aFlags)) {
           return NS_ERROR_OUT_OF_MEMORY;
         }
         if (mType == NS_FORM_INPUT_EMAIL) {
           UpdateAllValidityStates(!mDoneCreating);
         }
       } else {
         free(mInputData.mValue);
         mInputData.mValue = ToNewUnicode(value);
--- a/dom/html/HTMLInputElement.h
+++ b/dom/html/HTMLInputElement.h
@@ -937,19 +937,29 @@ protected:
                                      uint32_t aLen, uint32_t* aResult);
 
   // Helper method
 
   /**
    * Setting the value.
    *
    * @param aValue      String to set.
+   * @param aOldValue   Previous value before setting aValue.
+                        If previous value is unknown, aOldValue can be nullptr.
    * @param aFlags      See nsTextEditorState::SetValueFlags.
    */
-  nsresult SetValueInternal(const nsAString& aValue, uint32_t aFlags);
+  nsresult SetValueInternal(const nsAString& aValue,
+                            const nsAString* aOldValue,
+                            uint32_t aFlags);
+
+  nsresult SetValueInternal(const nsAString& aValue,
+                            uint32_t aFlags)
+  {
+    return SetValueInternal(aValue, nullptr, aFlags);
+  }
 
   // Generic getter for the value that doesn't do experimental control type
   // sanitization.
   void GetValueInternal(nsAString& aValue, CallerType aCallerType) const;
 
   // A getter for callers that know we're not dealing with a file input, so they
   // don't have to think about the caller type.
   void GetNonFileValueInternal(nsAString& aValue) const;
--- a/dom/html/nsTextEditorState.cpp
+++ b/dom/html/nsTextEditorState.cpp
@@ -2487,29 +2487,33 @@ nsTextEditorState::GetValue(nsAString& a
       mTextCtrlElement->GetDefaultValueFromContent(aValue);
     } else {
       aValue = *mValue;
     }
   }
 }
 
 bool
-nsTextEditorState::SetValue(const nsAString& aValue, uint32_t aFlags)
+nsTextEditorState::SetValue(const nsAString& aValue, const nsAString* aOldValue,
+                            uint32_t aFlags)
 {
   nsAutoString newValue(aValue);
 
   // While mIsCommittingComposition is true (that means that some event
   // handlers which are fired during committing composition are the caller of
   // this method), GetValue() uses mValueBeingSet for its result because the
   // first calls of this methods hasn't set the value yet.  So, when it's true,
   // we need to modify mValueBeingSet.  In this case, we will back to the first
   // call of this method, then, mValueBeingSet will be truncated when
   // mIsCommittingComposition is set false.  See below.
   if (mIsCommittingComposition) {
     mValueBeingSet = aValue;
+    // GetValue doesn't return current text frame's content during committing.
+    // So we cannot trust this old value
+    aOldValue = nullptr;
   }
 
   // Note that if this may be called during reframe of the editor.  In such
   // case, we shouldn't commit composition.  Therefore, when this is called
   // for internal processing, we shouldn't commit the composition.
   if (aFlags & (eSetValue_BySetUserInput | eSetValue_ByContent)) {
     if (EditorHasComposition()) {
       // When this is called recursively, there shouldn't be composition.
@@ -2520,24 +2524,35 @@ nsTextEditorState::SetValue(const nsAStr
         return true;
       }
       if (NS_WARN_IF(!mBoundFrame)) {
         // We're not sure if this case is possible.
       } else {
         // If setting value won't change current value, we shouldn't commit
         // composition for compatibility with the other browsers.
         nsAutoString currentValue;
-        mBoundFrame->GetText(currentValue);
+        if (aOldValue) {
+#ifdef DEBUG
+          mBoundFrame->GetText(currentValue);
+          MOZ_ASSERT(currentValue.Equals(*aOldValue));
+#endif
+          currentValue.Assign(*aOldValue);
+        } else {
+          mBoundFrame->GetText(currentValue);
+        }
         if (newValue == currentValue) {
           // Note that in this case, we shouldn't fire any events with setting
           // value because event handlers may try to set value recursively but
           // we cannot commit composition at that time due to unsafe to run
           // script (see below).
           return true;
         }
+        // IME might commit composition, then change value, so we cannot
+        // trust old value from parameter.
+        aOldValue = nullptr;
       }
       // If there is composition, need to commit composition first because
       // other browsers do that.
       // NOTE: We don't need to block nested calls of this because input nor
       //       other events won't be fired by setting values and script blocker
       //       is used during setting the value to the editor.  IE also allows
       //       to set the editor value on the input event which is caused by
       //       forcibly committing composition.
@@ -2588,17 +2603,25 @@ nsTextEditorState::SetValue(const nsAStr
 #ifdef DEBUG
     if (IsSingleLineTextControl()) {
       NS_ASSERTION(mEditorInitialized || mInitializing,
                    "We should never try to use the editor if we're not initialized unless we're being initialized");
     }
 #endif
 
     nsAutoString currentValue;
-    mBoundFrame->GetText(currentValue);
+    if (aOldValue) {
+#ifdef DEBUG
+      mBoundFrame->GetText(currentValue);
+      MOZ_ASSERT(currentValue.Equals(*aOldValue));
+#endif
+      currentValue.Assign(*aOldValue);
+    } else {
+      mBoundFrame->GetText(currentValue);
+    }
 
     AutoWeakFrame weakFrame(mBoundFrame);
 
     // this is necessary to avoid infinite recursion
     if (!currentValue.Equals(newValue))
     {
       ValueSetter valueSetter(mEditor);
 
--- a/dom/html/nsTextEditorState.h
+++ b/dom/html/nsTextEditorState.h
@@ -174,17 +174,24 @@ public:
     eSetValue_Notify                = 1 << 2,
     // Whether to move the cursor to end of the value (in the case when we have
     // cached selection offsets), in the case when the value has changed.  If
     // this is not set, the cached selection offsets will simply be clamped to
     // be within the length of the new value.  In either case, if the value has
     // not changed the cursor won't move.
     eSetValue_MoveCursorToEndIfValueChanged = 1 << 3,
   };
-  MOZ_MUST_USE bool SetValue(const nsAString& aValue, uint32_t aFlags);
+  MOZ_MUST_USE bool SetValue(const nsAString& aValue,
+                             const nsAString* aOldValue,
+                             uint32_t aFlags);
+  MOZ_MUST_USE bool SetValue(const nsAString& aValue,
+                             uint32_t aFlags)
+  {
+    return SetValue(aValue, nullptr, aFlags);
+  }
   void GetValue(nsAString& aValue, bool aIgnoreWrap) const;
   bool HasNonEmptyValue();
   // The following methods are for textarea element to use whether default
   // value or not.
   // XXX We might have to add assertion when it is into editable,
   // or reconsider fixing bug 597525 to remove these.
   void EmptyValue() { if (mValue) mValue->Truncate(); }
   bool IsEmpty() const { return mValue ? mValue->IsEmpty() : true; }