Fix update of validity cache value, so that it reflects the correct state of any FormControlElement.

Fix update of validity cache value, so that it reflects the correct
state of FormControlElements.

This patch does a lazy update of validity, avoiding caching stale value.
setNeedsValidityCheck() now does not update validity, but flags it for update.

BUG=400618, 403313
TEST=LayoutTests/fast/forms/number/number-validity-badinput.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181422

[spartha, 2014-08-14 05:32:15]

PTAL!!

[tkent, 2014-08-14 05:39:08]

https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
File Source/core/html/forms/NumberInputType.cpp (right):

https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
Source/core/html/forms/NumberInputType.cpp:240: // We need to pick the
visibleValue() for the check, if the editor is not yet synced.
I recommend to wrap code comments in 80 columns.

https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
Source/core/html/forms/NumberInputType.cpp:241: String standardValue =
convertFromVisibleValue(element().hasDirtyValue() &&
element().needsToUpdateViewValue() ? visibleValue() :
element().innerEditorValue());
Can we just add updateView() at the beginning of the function?

[spartha, 2014-08-14 08:03:20]

On 2014/08/14 05:39:08, tkent wrote:
>
https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
> File Source/core/html/forms/NumberInputType.cpp (right):
> 
>
https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
> Source/core/html/forms/NumberInputType.cpp:240: // We need to pick the
> visibleValue() for the check, if the editor is not yet synced.
> I recommend to wrap code comments in 80 columns.
> 
>
https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
> Source/core/html/forms/NumberInputType.cpp:241: String standardValue =
> convertFromVisibleValue(element().hasDirtyValue() &&
> element().needsToUpdateViewValue() ? visibleValue() :
> element().innerEditorValue());
> Can we just add updateView() at the beginning of the function?
updateView() is a non-const function. So I guess we cannot call it inside
hasBadInput.

[tkent, 2014-08-14 08:17:56]

On 2014/08/14 08:03:20, spartha wrote:
> > Can we just add updateView() at the beginning of the function?

> updateView() is a non-const function. So I guess we cannot call it inside
> hasBadInput.

I see.
Another idea is to add the following check at the beginning of the function.

    if (!element().hasDirtyValue())
        return false;

A number input can't have non-dirty badInput value because of value
sanitization.

[spartha, 2014-08-15 12:29:34]

Thanks for the review! PTAL

https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
File Source/core/html/forms/NumberInputType.cpp (right):

https://codereview.chromium.org/460343004/diff/60001/Source/core/html/forms/N...
Source/core/html/forms/NumberInputType.cpp:241: String standardValue =
convertFromVisibleValue(element().hasDirtyValue() &&
element().needsToUpdateViewValue() ? visibleValue() :
element().innerEditorValue());
On 2014/08/14 05:39:08, tkent wrote:
> Can we just add updateView() at the beginning of the function?

I have added the check at the beginning of the function, checking if the value
returned is an emptyString().

[tkent, 2014-08-15 13:02:00]

I'm sorry that I misunderstood hasDirtyValue.  I thought it returned false when
the value was updated by input.value = '1'.

Patch Set 2 doesn't look correct, and I still think we have to call updateView()
somewhere.
Would you investigate why HTMLInputElement::setValue doesn't call
setInnerEditorValue in TextFieldInputType::updateView() please?

[spartha, 2014-08-15 13:40:44]

On 2014/08/15 13:02:00, tkent wrote:
> I'm sorry that I misunderstood hasDirtyValue.  I thought it returned false
when
> the value was updated by input.value = '1'.
> 
> Patch Set 2 doesn't look correct, and I still think we have to call
updateView()
> somewhere.
> Would you investigate why HTMLInputElement::setValue doesn't call
> setInnerEditorValue in TextFieldInputType::updateView() please?

As far as I remember, calling updateView() without changing hasBadInput, will
fail
in assertion in debug builds. This was because, setInnerEditorValue would cause
style recalc, to determine pseudo:invalid state etc. which in turn calls
hasBadInput
via  HTMLFormControlElement::isValidFormControlElement()

[spartha, 2014-08-15 13:54:09]

On 2014/08/15 13:40:44, spartha wrote:
> On 2014/08/15 13:02:00, tkent wrote:
> > I'm sorry that I misunderstood hasDirtyValue.  I thought it returned false
> when
> > the value was updated by input.value = '1'.
> > 
> > Patch Set 2 doesn't look correct, and I still think we have to call
> updateView()
> > somewhere.
> > Would you investigate why HTMLInputElement::setValue doesn't call
> > setInnerEditorValue in TextFieldInputType::updateView() please?
> 
> As far as I remember, calling updateView() without changing hasBadInput, will
> fail
> in assertion in debug builds. This was because, setInnerEditorValue would
cause
> style recalc, to determine pseudo:invalid state etc. which in turn calls
> hasBadInput
> via  HTMLFormControlElement::isValidFormControlElement()

Calling updateView() at the beginning of checkValidity() may be one option. I
will make the change and then update further

[tkent, 2014-08-15 14:11:58]

On 2014/08/15 13:54:09, spartha wrote:
> On 2014/08/15 13:40:44, spartha wrote:
> > On 2014/08/15 13:02:00, tkent wrote:
> > > I'm sorry that I misunderstood hasDirtyValue.  I thought it returned false
> > when
> > > the value was updated by input.value = '1'.
> > > 
> > > Patch Set 2 doesn't look correct, and I still think we have to call
> > updateView()
> > > somewhere.
> > > Would you investigate why HTMLInputElement::setValue doesn't call
> > > setInnerEditorValue in TextFieldInputType::updateView() please?
> > 
> > As far as I remember, calling updateView() without changing hasBadInput,
will
> > fail
> > in assertion in debug builds. This was because, setInnerEditorValue would
> cause
> > style recalc, to determine pseudo:invalid state etc. which in turn calls
> > hasBadInput
> > via  HTMLFormControlElement::isValidFormControlElement()
> 
> Calling updateView() at the beginning of checkValidity() may be one option. I
> will make the change and then update further

I don't think so.
We should call updateView somewhere inside HTMLInputElement::setValue.  It is
usually called. Please read the comment #7.
> Would you investigate why HTMLInputElement::setValue doesn't call
> setInnerEditorValue in TextFieldInputType::updateView() please?

[spartha, 2014-08-16 05:21:21]

On 2014/08/15 14:11:58, tkent wrote:
> On 2014/08/15 13:54:09, spartha wrote:
> > On 2014/08/15 13:40:44, spartha wrote:
> > > On 2014/08/15 13:02:00, tkent wrote:
> > > > I'm sorry that I misunderstood hasDirtyValue.  I thought it returned
false
> > > when
> > > > the value was updated by input.value = '1'.
> > > > 
> > > > Patch Set 2 doesn't look correct, and I still think we have to call
> > > updateView()
> > > > somewhere.
> > > > Would you investigate why HTMLInputElement::setValue doesn't call
> > > > setInnerEditorValue in TextFieldInputType::updateView() please?
> > > 
> > > As far as I remember, calling updateView() without changing hasBadInput,
> will
> > > fail
> > > in assertion in debug builds. This was because, setInnerEditorValue would
> > cause
> > > style recalc, to determine pseudo:invalid state etc. which in turn calls
> > > hasBadInput
> > > via  HTMLFormControlElement::isValidFormControlElement()
> > 
> > Calling updateView() at the beginning of checkValidity() may be one option.
I
> > will make the change and then update further
> 
> I don't think so.
> We should call updateView somewhere inside HTMLInputElement::setValue.  It is
> usually called. Please read the comment #7.
> > Would you investigate why HTMLInputElement::setValue doesn't call
> > setInnerEditorValue in TextFieldInputType::updateView() please?

There is this function NumberInputType::setValue, which finally gets called for
setValue. This calls updateView() only on s

   if (!valueChanged && sanitizedValue.isEmpty() &&
!element().innerEditorValue().isEmpty())
        element().updateView();
I could not get the history for this change and I am not sure why this check has
been added.

[tkent, 2014-08-18 01:59:57]

On 2014/08/16 05:21:21, spartha wrote:
> There is this function NumberInputType::setValue, which finally gets called
for
> setValue. This calls updateView() only on s
> 
>    if (!valueChanged && sanitizedValue.isEmpty() &&
> !element().innerEditorValue().isEmpty())
>         element().updateView();
> I could not get the history for this change and I am not sure why this check
has
> been added.

I don't think this updateView() should be called.  Is updateView() in
TextFieldInputType::setValue called?

[spartha, 2014-08-18 14:31:33]

On 2014/08/18 01:59:57, tkent wrote:
> On 2014/08/16 05:21:21, spartha wrote:
> > There is this function NumberInputType::setValue, which finally gets called
> for
> > setValue. This calls updateView() only on s
> > 
> >    if (!valueChanged && sanitizedValue.isEmpty() &&
> > !element().innerEditorValue().isEmpty())
> >         element().updateView();
> > I could not get the history for this change and I am not sure why this check
> has
> > been added.
> 
> I don't think this updateView() should be called.  Is updateView() in
> TextFieldInputType::setValue called?

Yes. But by then, the validity is already cached with the old text.

[spartha, 2014-08-19 13:05:42]

With this patch, I now call updateView() for InputType textfield before the call
to setNeedsValidityCheck(). This addresses the crash in Debug builds and the
incorrect validity value. PTAL, thanks!

[tkent, 2014-08-19 22:53:48]

On 2014/08/19 13:05:42, spartha wrote:
> With this patch, I now call updateView() for InputType textfield before the
call
> to setNeedsValidityCheck(). This addresses the crash in Debug builds and the
> incorrect validity value. PTAL, thanks!

Adding updateView call has performance issue. 
- Adding setNeedsValidityCheck in TextFieldInputType::setValue, or
- Moving setNeedsValidityCheck in HTMLInputElement::setValueInternal to
HTMLInputElement::setValue
would be better.

[spartha, 2014-08-20 05:53:12]

On 2014/08/19 22:53:48, tkent wrote:
> On 2014/08/19 13:05:42, spartha wrote:
> > With this patch, I now call updateView() for InputType textfield before the
> call
> > to setNeedsValidityCheck(). This addresses the crash in Debug builds and the
> > incorrect validity value. PTAL, thanks!
> 
> Adding updateView call has performance issue. 
> - Adding setNeedsValidityCheck in TextFieldInputType::setValue, or
I have tried this, but this asserts in debug builds, as there would 
still be a mismatch between the cached validity value and the actual
value.

> - Moving setNeedsValidityCheck in HTMLInputElement::setValueInternal to
> HTMLInputElement::setValue
> would be better.
This I can try. The reason I called updateView in setValueInternal, was that I
saw 
there was already checks in TextFieldInputType::updateView, to not update the
view unnecessarily.

[spartha, 2014-08-20 08:34:48]

On 2014/08/20 05:53:12, spartha wrote:
> On 2014/08/19 22:53:48, tkent wrote:
> > On 2014/08/19 13:05:42, spartha wrote:
> > > With this patch, I now call updateView() for InputType textfield before
the
> > call
> > > to setNeedsValidityCheck(). This addresses the crash in Debug builds and
the
> > > incorrect validity value. PTAL, thanks!
> > 
> > Adding updateView call has performance issue. 
> > - Adding setNeedsValidityCheck in TextFieldInputType::setValue, or
> I have tried this, but this asserts in debug builds, as there would 
> still be a mismatch between the cached validity value and the actual
> value.
> 
> > - Moving setNeedsValidityCheck in HTMLInputElement::setValueInternal to
> > HTMLInputElement::setValue
> > would be better.
> This I can try. The reason I called updateView in setValueInternal, was that I
> saw 
> there was already checks in TextFieldInputType::updateView, to not update the
> view unnecessarily.

Unfortunately, both your proposed solutions have the same problem in Debug
builds. They still have a stale cached value, when the style update triggers a
call to isValidFormControlElement causing it to fail assertion. Could we stick
to the last patch, as I feel there are sufficient checks in place to prevent
updateView call from being expensive.

[spartha, 2014-08-20 08:59:07]

PatchSet 4 adds additional check for calling updateView, in view of the concerns
raised. This will ensure that updateView is called only if HTMLInput::setValue
is called. PTAL, thanks!

[tkent, 2014-08-21 01:54:15]

On 2014/08/20 08:34:48, spartha wrote:
> Unfortunately, both your proposed solutions have the same problem in Debug
> builds. They still have a stale cached value, when the style update triggers a
> call to isValidFormControlElement causing it to fail assertion.

Please explain the detail.  When does the style recalc happen?

> Could we stick
> to the last patch, as I feel there are sufficient checks in place to prevent
> updateView call from being expensive.

No.  Adding another udpateView hides the problem, doesn't resolve the problem.

[spartha, 2014-08-26 08:47:00]

Patchset #6 (id:160001) has been deleted

[spartha, 2014-08-26 09:07:36]

Refactoring the code to address the issue. Text Inputs rely on inner editor
value for checking input validity. Hence when text inputs set value, they need
to do an intermediate step of updating their view, before validation. To achieve
this, the patch 
1. Takes out validity check out of HTMLInputElement::setValueInternal.
2. Provides a convenience method
HTMLInputElement::setValueInternalAndUpdateValidity for other input types that
don't need to do the intermediate step
3. Modifies TextFieldInputType::setValue to handle the case for text fields.
4. Rename setNeedsValidityCheck to updateValidity, for better readability and to
reflect the actual functionality.

PTAL!

[tkent, 2014-08-26 22:30:19]

On 2014/08/21 01:54:15, tkent wrote:
> On 2014/08/20 08:34:48, spartha wrote:
> > Unfortunately, both your proposed solutions have the same problem in Debug
> > builds. They still have a stale cached value, when the style update triggers
a
> > call to isValidFormControlElement causing it to fail assertion.
> 
> Please explain the detail.  When does the style recalc happen?

Please answer this.

> 4. Rename setNeedsValidityCheck to updateValidity, for better readability and
to
reflect the actual functionality.

You shouldn't do it in this CL.  It's unrelated to the bug.

[spartha, 2014-08-27 03:11:51]

On 2014/08/26 22:30:19, tkent wrote:
> On 2014/08/21 01:54:15, tkent wrote:
> > On 2014/08/20 08:34:48, spartha wrote:
> > > Unfortunately, both your proposed solutions have the same problem in Debug
> > > builds. They still have a stale cached value, when the style update
triggers
> a
> > > call to isValidFormControlElement causing it to fail assertion.
> > 
> > Please explain the detail.  When does the style recalc happen?
> 
> Please answer this.

There are a couple of places where the need for style recalc is set in the
setValue flow currently primarily to update the PseudoInvalid/PseudoValid
states. One is from InputType::setValue and the other is from
setNeedsValidityCheck(). isValidFormControlElement is called during the
determination of PseudoInvalid/PseudoValid states. Any call to it after setValue
and a subsequent call to setNeedsValidityCheck(), is bound to fail the
assertion, as the cached value and the actual input value are not the same.

> 
> > 4. Rename setNeedsValidityCheck to updateValidity, for better readability
and
> to
> reflect the actual functionality.
> 
> You shouldn't do it in this CL.  It's unrelated to the bug.

ok. I will add a separate CL for this.

[tkent, 2014-08-27 03:46:07]

On 2014/08/27 03:11:51, spartha wrote:
> On 2014/08/26 22:30:19, tkent wrote:
> > On 2014/08/21 01:54:15, tkent wrote:
> > > On 2014/08/20 08:34:48, spartha wrote:
> > > > Unfortunately, both your proposed solutions have the same problem in
Debug
> > > > builds. They still have a stale cached value, when the style update
> triggers
> > a
> > > > call to isValidFormControlElement causing it to fail assertion.
> > > 
> > > Please explain the detail.  When does the style recalc happen?
> > 
> > Please answer this.
> 
> There are a couple of places where the need for style recalc is set in the
> setValue flow currently primarily to update the PseudoInvalid/PseudoValid
> states. One is from InputType::setValue and the other is from
> setNeedsValidityCheck(). isValidFormControlElement is called during the
> determination of PseudoInvalid/PseudoValid states. Any call to it after
setValue
> and a subsequent call to setNeedsValidityCheck(), is bound to fail the
> assertion, as the cached value and the actual input value are not the same.

Do you mean setNeedsStyleRecalc call in setNeedsValidityCheck triggers the
assertion failure?
Please show the stack trace of the assertion failure.

[spartha, 2014-08-27 03:48:40]

On 2014/08/27 03:46:07, tkent wrote:
> On 2014/08/27 03:11:51, spartha wrote:
> > On 2014/08/26 22:30:19, tkent wrote:
> > > On 2014/08/21 01:54:15, tkent wrote:
> > > > On 2014/08/20 08:34:48, spartha wrote:
> > > > > Unfortunately, both your proposed solutions have the same problem in
> Debug
> > > > > builds. They still have a stale cached value, when the style update
> > triggers
> > > a
> > > > > call to isValidFormControlElement causing it to fail assertion.
> > > > 
> > > > Please explain the detail.  When does the style recalc happen?
> > > 
> > > Please answer this.
> > 
> > There are a couple of places where the need for style recalc is set in the
> > setValue flow currently primarily to update the PseudoInvalid/PseudoValid
> > states. One is from InputType::setValue and the other is from
> > setNeedsValidityCheck(). isValidFormControlElement is called during the
> > determination of PseudoInvalid/PseudoValid states. Any call to it after
> setValue
> > and a subsequent call to setNeedsValidityCheck(), is bound to fail the
> > assertion, as the cached value and the actual input value are not the same.
> 
> Do you mean setNeedsStyleRecalc call in setNeedsValidityCheck triggers the
> assertion failure?
> Please show the stack trace of the assertion failure.

[spartha, 2014-08-27 03:55:13]

On 2014/08/27 03:48:40, spartha wrote:
> On 2014/08/27 03:46:07, tkent wrote:
> > On 2014/08/27 03:11:51, spartha wrote:
> > > On 2014/08/26 22:30:19, tkent wrote:
> > > > On 2014/08/21 01:54:15, tkent wrote:
> > > > > On 2014/08/20 08:34:48, spartha wrote:
> > > > > > Unfortunately, both your proposed solutions have the same problem in
> > Debug
> > > > > > builds. They still have a stale cached value, when the style update
> > > triggers
> > > > a
> > > > > > call to isValidFormControlElement causing it to fail assertion.
> > > > > 
> > > > > Please explain the detail.  When does the style recalc happen?
> > > > 
> > > > Please answer this.
> > > 
> > > There are a couple of places where the need for style recalc is set in the
> > > setValue flow currently primarily to update the PseudoInvalid/PseudoValid
> > > states. One is from InputType::setValue and the other is from
> > > setNeedsValidityCheck(). isValidFormControlElement is called during the
> > > determination of PseudoInvalid/PseudoValid states. Any call to it after
> > setValue
> > > and a subsequent call to setNeedsValidityCheck(), is bound to fail the
> > > assertion, as the cached value and the actual input value are not the
same.
> > 
> > Do you mean setNeedsStyleRecalc call in setNeedsValidityCheck triggers the
> > assertion failure?
> > Please show the stack trace of the assertion failure.
Possibly. But InputType::setValue certainly would as the setNeedsStyleRecalc is
unconditionally called. I will add the stack trace to the bug.

[spartha, 2014-08-27 06:06:04]

On 2014/08/27 03:55:13, spartha wrote:
> On 2014/08/27 03:48:40, spartha wrote:
> > On 2014/08/27 03:46:07, tkent wrote:
> > > On 2014/08/27 03:11:51, spartha wrote:
> > > > On 2014/08/26 22:30:19, tkent wrote:
> > > > > On 2014/08/21 01:54:15, tkent wrote:
> > > > > > On 2014/08/20 08:34:48, spartha wrote:
> > > > > > > Unfortunately, both your proposed solutions have the same problem
in
> > > Debug
> > > > > > > builds. They still have a stale cached value, when the style
update
> > > > triggers
> > > > > a
> > > > > > > call to isValidFormControlElement causing it to fail assertion.
> > > > > > 
> > > > > > Please explain the detail.  When does the style recalc happen?
> > > > > 
> > > > > Please answer this.
> > > > 
> > > > There are a couple of places where the need for style recalc is set in
the
> > > > setValue flow currently primarily to update the
PseudoInvalid/PseudoValid
> > > > states. One is from InputType::setValue and the other is from
> > > > setNeedsValidityCheck(). isValidFormControlElement is called during the
> > > > determination of PseudoInvalid/PseudoValid states. Any call to it after
> > > setValue
> > > > and a subsequent call to setNeedsValidityCheck(), is bound to fail the
> > > > assertion, as the cached value and the actual input value are not the
> same.
> > > 
> > > Do you mean setNeedsStyleRecalc call in setNeedsValidityCheck triggers the
> > > assertion failure?
> > > Please show the stack trace of the assertion failure.
> Possibly. But InputType::setValue certainly would as the setNeedsStyleRecalc
is
> unconditionally called. I will add the stack trace to the bug.
There are many paths that can cause the assertion to fail. This one I got by the
following snippet

<input type=number id=number>
var number = document.getElementById('number');
number.focus();
document.execCommand('InsertText', false, 'a');
number.value = 1;

The point I am trying to make is that, the root cause is the stale value of
validity Cache.

#0 0x7fcdf7293cfe base::debug::StackTrace::StackTrace()
#1 0x7fcdf7293830 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fcde804d340 <unknown>
#3 0x7fcdee755d41 blink::HTMLFormControlElement::isValidFormControlElement()
#4 0x7fcdeeaf0dde blink::SelectorChecker::checkOne<>()
#5 0x7fcdeeaef27e blink::SelectorChecker::match<>()
#6 0x7fcdeeaa1864 blink::ElementRuleCollector::ruleMatches()
#7 0x7fcdeeaa03f8 blink::ElementRuleCollector::collectRuleIfMatches()
#8 0x7fcdeeaa0c1c blink::ElementRuleCollector::collectMatchingRulesForList<>()
#9 0x7fcdeea9fcef blink::ElementRuleCollector::collectMatchingRules()
#10 0x7fcdeeb71d1c blink::ScopedStyleResolver::collectMatchingAuthorRules()
#11 0x7fcdeeb8c7d8 blink::StyleResolver::matchAuthorRules()
#12 0x7fcdeeb8cfe6 blink::StyleResolver::matchAllRules()
#13 0x7fcdeeb8dce1 blink::StyleResolver::styleForElement()
#14 0x7fcdee4d3225 blink::Element::originalStyleForRenderer()
#15 0x7fcdee776b80 blink::HTMLInputElement::customStyleForRenderer()
#16 0x7fcdee4d2f7e blink::Element::styleForRenderer()
#17 0x7fcdee4d368c blink::Element::recalcOwnStyle()
#18 0x7fcdee4d3409 blink::Element::recalcStyle()
#19 0x7fcdee4d3d8e blink::Element::recalcChildStyle()
#20 0x7fcdee4d3457 blink::Element::recalcStyle()
#21 0x7fcdee4d3d8e blink::Element::recalcChildStyle()
#22 0x7fcdee4d3457 blink::Element::recalcStyle()
#23 0x7fcdee4d3d8e blink::Element::recalcChildStyle()
#24 0x7fcdee4d3457 blink::Element::recalcStyle()
#25 0x7fcdee473b90 blink::Document::updateStyle()
#26 0x7fcdee4734be blink::Document::updateRenderTree()
#27 0x7fcdecdc76ba blink::Document::updateRenderTreeIfNeeded()
#28 0x7fcdee47426d blink::Document::updateLayout()
#29 0x7fcdeec488c8 blink::VisibleSelection::toNormalizedRange()
#30 0x7fcdeec28cc2 blink::SpellChecker::respondToChangedSelection()
#31 0x7fcdeebe4fa2 blink::Editor::respondToChangedSelection()
#32 0x7fcdeebf3f0e blink::FrameSelection::setSelection()
#33 0x7fcdeebf69fc blink::FrameSelection::updateSelectionIfNeeded()
#34 0x7fcdeebf65a2 blink::FrameSelection::didUpdateCharacterData()
#35 0x7fcdee43e6b2 blink::CharacterData::setDataAndUpdate()
#36 0x7fcdee43e4e4 blink::CharacterData::setData()
#37 0x7fcdeec60bd4 blink::replaceChildrenWithText()
#38 0x7fcdee74acba blink::HTMLElement::setInnerText()
#39 0x7fcdee7d36a8 blink::HTMLTextFormControlElement::setInnerEditorValue()
#40 0x7fcdee7731b7 blink::HTMLInputElement::setInnerEditorValue()
#41 0x7fcdee897e2a blink::TextFieldInputType::updateView()
#42 0x7fcdee7733a0 blink::HTMLInputElement::updateView()
#43 0x7fcdee895a42 blink::TextFieldInputType::setValue()
#44 0x7fcdee8882a5 blink::NumberInputType::setValue()
#45 0x7fcdee7729a0 blink::HTMLInputElement::setValue()
#46 0x7fcdee77327e blink::HTMLInputElement::setValue()
#47 0x7fcdedbfa2e3 blink::HTMLInputElementV8Internal::valueAttributeSetter()
#48 0x7fcdedbf82e1
blink::HTMLInputElementV8Internal::valueAttributeSetterCallback()
#49 0x7fcdf8dd22c4 v8::internal::PropertyCallbackArguments::Call()
#50 0x7fcdf90e7151 v8::internal::Object::SetPropertyWithAccessor()
#51 0x7fcdf90fe449 v8::internal::Object::SetProperty()
#52 0x7fcdf90fddcf v8::internal::Object::SetProperty()
#53 0x7fcdf906290e v8::internal::StoreIC::Store()
#54 0x7fcdf906771d v8::internal::StoreIC_Miss()

[tkent, 2014-08-28 08:47:30]

The stack trace is interesting.  So, we can't resolve this issue by reordering
updateView and setNeedsValidityCheck.

[spartha, 2014-08-28 10:40:09]

On 2014/08/28 08:47:30, tkent wrote:
> The stack trace is interesting.  So, we can't resolve this issue by reordering
> updateView and setNeedsValidityCheck.

I was thinking in the lines of having a dirty flag, and we can do a validity
check when needed. Can your idea of doing a lazy validity check help here?

[tkent, 2014-09-01 02:33:05]

On 2014/08/28 10:40:09, spartha wrote:
> On 2014/08/28 08:47:30, tkent wrote:
> > The stack trace is interesting.  So, we can't resolve this issue by
reordering
> > updateView and setNeedsValidityCheck.
> 
> I was thinking in the lines of having a dirty flag, and we can do a validity
> check when needed. Can your idea of doing a lazy validity check help here?

That's exactly the idea of the lazy evaluation.
* setNeedsValidityCheck sets HTMLFormControlElement::m_validityIsDirty.
* HTMLFormControlElement::isValidFormControlElement() should do |m_isValid =
valid(); m_validityIsDirty=false;| if m_validityIsDirty.
* We need to call setNeedsValidityCheck before updateView for this bug.

[tkent, 2014-09-01 02:34:44]

On 2014/09/01 02:33:05, tkent wrote:
> On 2014/08/28 10:40:09, spartha wrote:
> > On 2014/08/28 08:47:30, tkent wrote:
> > > The stack trace is interesting.  So, we can't resolve this issue by
> reordering
> > > updateView and setNeedsValidityCheck.
> > 
> > I was thinking in the lines of having a dirty flag, and we can do a validity
> > check when needed. Can your idea of doing a lazy validity check help here?
> 
> That's exactly the idea of the lazy evaluation.
> * setNeedsValidityCheck sets HTMLFormControlElement::m_validityIsDirty.
> * HTMLFormControlElement::isValidFormControlElement() should do |m_isValid =
> valid(); m_validityIsDirty=false;| if m_validityIsDirty.
> * We need to call setNeedsValidityCheck before updateView for this bug.

We don't have a plan to do it in short term.  It would be helpful if you work on
this change.

[spartha, 2014-09-02 14:09:55]

This patch tries to address the lazy validity check. PTAL!

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (left):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:333: updateValue();
Replaced by a call from subTreeChanged()

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (left):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:131: bool m_valueIsUpToDate;
The flag is set in subTreeChanged() and the value was updated in the same call
stack (in the call to valid()). So there was no lazy update happening as such.
This flag has been replaced by a direct call to updateValue(). This change was
needed as we do a lazy update of validity.

[tkent, 2014-09-03 02:16:36]

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:61: , m_validIsDirty(false)
Initial value should be |true| conceptually.

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:488: if (willValidate())
Do we need this check?  When !willValidate(), m_isValid has stale value.

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.h (right):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.h:184: bool m_validIsDirty : 1;
The name is weird.
'valid' is an adjective.  It should't be a subject of a sentence.  This should
be 'm_validityIsDirty'

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (left):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:131: bool m_valueIsUpToDate;
On 2014/09/02 14:09:55, spartha wrote:
> The flag is set in subTreeChanged() and the value was updated in the same call
> stack (in the call to valid()). So there was no lazy update happening as such.
> This flag has been replaced by a direct call to updateValue(). This change was
> needed as we do a lazy update of validity.   

Don't change this.  The change will cause performance regression.

[spartha, 2014-09-03 06:14:47]

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (left):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:131: bool m_valueIsUpToDate;
On 2014/09/03 02:16:35, tkent wrote:
> On 2014/09/02 14:09:55, spartha wrote:
> > The flag is set in subTreeChanged() and the value was updated in the same
call
> > stack (in the call to valid()). So there was no lazy update happening as
such.
> > This flag has been replaced by a direct call to updateValue(). This change
was
> > needed as we do a lazy update of validity.   
> 
> Don't change this.  The change will cause performance regression.

I have not come across m_valueIsUpToDate being set to false anywhere else, hence
I don't see it doing a lazy update. The call to setNeedsValidtyCheck() in
subTreeChanged() would update the value anyway. Have I missed any other
alternate flow that would benefit from this setup?

[tkent, 2014-09-03 07:56:43]

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (left):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:131: bool m_valueIsUpToDate;
On 2014/09/03 06:14:47, spartha wrote:
> On 2014/09/03 02:16:35, tkent wrote:
> > On 2014/09/02 14:09:55, spartha wrote:
> > > The flag is set in subTreeChanged() and the value was updated in the same
> call
> > > stack (in the call to valid()). So there was no lazy update happening as
> such.
> > > This flag has been replaced by a direct call to updateValue(). This change
> was
> > > needed as we do a lazy update of validity.   
> > 
> > Don't change this.  The change will cause performance regression.
> 
> I have not come across m_valueIsUpToDate being set to false anywhere else,
hence
> I don't see it doing a lazy update. The call to setNeedsValidtyCheck() in
> subTreeChanged() would update the value anyway. Have I missed any other
> alternate flow that would benefit from this setup? 

- If willValidate() is false, updateValue() shouldn't be called immediately.
- By the lazy evaluation of validity, the number of updateValue() calls would be
reduced. We'll have performance benefit.

[spartha, 2014-09-03 07:58:40]

Alternate approach. PTAL!

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:61: , m_validIsDirty(false)
On 2014/09/03 02:16:35, tkent wrote:
> Initial value should be |true| conceptually.
> 

Done.

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:488: if (willValidate())
On 2014/09/03 02:16:35, tkent wrote:
> Do we need this check?  When !willValidate(), m_isValid has stale value.

Done.

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.h (right):

https://codereview.chromium.org/460343004/diff/240001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.h:184: bool m_validIsDirty : 1;
On 2014/09/03 02:16:35, tkent wrote:
> The name is weird.
> 'valid' is an adjective.  It should't be a subject of a sentence.  This should
> be 'm_validityIsDirty'

Done.

[tkent, 2014-09-03 07:58:47]

If a textarea has neither |required| attribute nor |maxlength| attribute,
validation process should not call value(). However, it calls for now.  It's a
bug.

[tkent, 2014-09-03 08:04:44]

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:490: }
We should keep the ASSERT if !m_validityIsDirty.

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:579: // while saving the form control
state.
I don't understand the comment.
Does the code to save form control value use m_value instead of value()?

[spartha, 2014-09-03 08:50:58]

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:490: }
On 2014/09/03 08:04:44, tkent wrote:
> We should keep the ASSERT if !m_validityIsDirty.

Wouldn't two calls to element.checkValidity() cause the assert to fail?

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:579: // while saving the form control
state.
On 2014/09/03 08:04:44, tkent wrote:
> I don't understand the comment.
> Does the code to save form control value use m_value instead of value()?
It uses value(). It tries to update m_value in the process. This causes the
assertion for renderer() to fail, as the element is already detached. I guess
this issue got masked by the call to setNeedsValidityCheck in subTreeChanged()
which would call value().

#0 0x7f17d2e2dcfe base::debug::StackTrace::StackTrace()
#1 0x7f17d2e2d830 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f17c3be7340 <unknown>
#3 0x7f17ca368c16 blink::HTMLTextAreaElement::updateValue()
#4 0x7f17ca367a17 blink::HTMLTextAreaElement::value()
#5 0x7f17ca3679af blink::HTMLTextAreaElement::saveFormControlState()
#6 0x7f17ca40bf3c blink::DocumentState::toStateVector()
#7 0x7f17caa125de blink::HistoryItem::documentState()
#8 0x7f17caa12633 blink::HistoryItem::getReferencedFilePaths()
#9 0x7f17c89a4202 blink::WebHistoryItem::getReferencedFilePaths()
#10 0x7f17cda40bf7 content::HistoryEntryToPageState()
#11 0x7f17cdb8837e content::RenderViewImpl::SendUpdateState()
#12 0x7f17cdb882a3 content::RenderViewImpl::UpdateSessionHistory()
#13 0x7f17cdb37ee2 content::RenderFrameImpl::didCommitProvisionalLoad()
#14 0x00000056e380 content::WebFrameTestProxy<>::didCommitProvisionalLoad()
#15 0x00000056f3ed content::WebFrameTestProxy<>::didCommitProvisionalLoad()
#16 0x7f17c89250ec blink::FrameLoaderClientImpl::dispatchDidCommitLoad()
#17 0x7f17caa0ab79 blink::FrameLoader::receivedFirstData()
#18 0x7f17ca9fb5fb blink::DocumentLoader::ensureWriter()
#19 0x7f17ca9f9d55 blink::DocumentLoader::commitData()
#20 0x7f17ca9f9922 blink::DocumentLoader::finishedLoading()
#21 0x7f17ca9fc686 blink::DocumentLoader::maybeLoadEmpty()
#22 0x7f17ca9fc7f9 blink::DocumentLoader::startLoadingMainResource()
#23 0x7f17caa0ddb9 blink::FrameLoader::loadWithNavigationAction()
#24 0x7f17caa0cfa3 blink::FrameLoader::load()

[tkent, 2014-09-03 09:11:27]

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:490: }
On 2014/09/03 08:50:57, spartha wrote:
> On 2014/09/03 08:04:44, tkent wrote:
> > We should keep the ASSERT if !m_validityIsDirty.
> 
> Wouldn't two calls to element.checkValidity() cause the assert to fail?

I don't think so.

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:579: // while saving the form control
state.
Thanks.  I understand.
We should remove ASSERT(renderer()).  It's bogus now.

[spartha, 2014-09-03 14:47:33]

PTAL!

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:490: }
On 2014/09/03 09:11:27, tkent wrote:
> On 2014/09/03 08:50:57, spartha wrote:
> > On 2014/09/03 08:04:44, tkent wrote:
> > > We should keep the ASSERT if !m_validityIsDirty.
> > 
> > Wouldn't two calls to element.checkValidity() cause the assert to fail?
> 
> I don't think so.
Done. You are right. I missed the placement of the assert.

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/460343004/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:579: // while saving the form control
state.
On 2014/09/03 09:11:27, tkent wrote:
> Thanks.  I understand.
> We should remove ASSERT(renderer()).  It's bogus now.

Done.

[tkent, 2014-09-04 04:49:36]

https://codereview.chromium.org/460343004/diff/320001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/320001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:481: ASSERT(!m_validityIsDirty);
This assertion doesn't make sense.
I meant:

if (m_validityIsDirty) {
    ....
} else {
    // If the following assertions fails, ...
    ASSERT(m_isValid == valid());
}

[spartha, 2014-09-04 05:20:57]

https://codereview.chromium.org/460343004/diff/320001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/460343004/diff/320001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:481: ASSERT(!m_validityIsDirty);
On 2014/09/04 04:49:36, tkent wrote:
> This assertion doesn't make sense.
> I meant:
> 
> if (m_validityIsDirty) {
>     ....
> } else {
>     // If the following assertions fails, ...
>     ASSERT(m_isValid == valid());
> }

Done.

[tkent, 2014-09-04 12:41:04]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-04 12:41:07]

lgtm

[commit-bot: I haz the power, 2014-09-04 12:41:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/460343004/340001

[commit-bot: I haz the power, 2014-09-04 15:30:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:30:08]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25561)

[spartha, 2014-09-04 20:09:05]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-09-04 20:10:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/460343004/340001

[commit-bot: I haz the power, 2014-09-05 00:09:40]

Committed patchset #14 (id:340001) as 181422

[spartha, 2014-09-20 06:30:31]

A revert of this CL (patchset #14 id:340001) has been created in
https://codereview.chromium.org/583833007/ by sudarshan.p@samsung.com.

The reason for reverting is: Caused regression in blink_perf. crbug.com/412054 .