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
--- 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 : ¤tValue,
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; }