Bug 1368888 - Don't get previous value twice in input.value setter. r?smaug draft
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Wed, 14 Jun 2017 18:21:01 +0900
changeset 595331 aa45f0419faf7982c973478a411cdec147421ccf
parent 593717 b266a8d8fd595b84a7d6218d7b8c6b7af0b5027c
child 633670 c0d4fde04cacc2601bec4aaf3045d9e69a8b96dc
push id64296
push userm_kato@ga2.so-net.ne.jp
push dateFri, 16 Jun 2017 07:05:30 +0000
reviewerssmaug
bugs1368888
milestone56.0a1
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
@@ -935,19 +935,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; }