Bug 45681 – Assertion failure: m_isValid == validity()->valid() after manipulating a detached element

• Component: Forms
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Created attachment 67422 [details]
test case (may crash)

See attached test case.

[Kent Tamura]

Created attachment 67493 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 67493 [details]
Patch

Why do we need to call setNeedsValidityCheck() in both functions? One of these always calls another.

If we do need that, it may be worth a comment in code, or at the very least, in ChangeLog.

[Kent Tamura]

Thank you for the feedback.

(In reply to comment #2)
> (From update of attachment 67493 [details])
> Why do we need to call setNeedsValidityCheck() in both functions? One of these always calls another.
> 
> If we do need that, it may be worth a comment in code, or at the very least, in ChangeLog.

setNonDirtyValue() is called by other functions. We need to call setNeedsValidityCheck() in that case.
I'll improve the code to reduce redundant calls of setNeedsValidityCheck().

[Kent Tamura]

Created attachment 67754 [details]
Patch 2 (resolve redundant setNeedsValidityCheck())

[Alexey Proskuryakov]

> setNonDirtyValue() is called by other functions.

OK, but why wouldn't it be sufficient to only have setNeedsValidityCheck() in setNonDirtyValue()? Since setValue() calls it, it doesn't seem to also call the same function.

Talking about the new patch - this is probably a related question, but why can't setNeedsValidityCheck() be in setValueCommon()? Does its behavior depend on m_isDirty value? It doesn't use it explicitly, and if an indirect use is the reason not to put this call in setValueCommon(), then a comment should explain that (and setNeedsValidityCheck() probably needs a better name, since it does something different than setting a "needsValidityCheck" boolean).

Both these patches confuse me, but it seems that a larger refactoring may be needed. If so, the first one might actually be better as a targeted fix, as it's simpler and doesn't add a new method.

[Kent Tamura]

(In reply to comment #5)
> > setNonDirtyValue() is called by other functions.
> 
> OK, but why wouldn't it be sufficient to only have setNeedsValidityCheck() in setNonDirtyValue()? Since setValue() calls it, it doesn't seem to also call the same function.
> 
> but why can't setNeedsValidityCheck() be in setValueCommon()? Does its behavior depend on m_isDirty value?

Right.  setNeedsValidityCheck() result depends on m_isDirty value.

The name "setNeedsValidityCheck" doesn't represent the current behavior.  The role of setNeedsValdityCheck was to set "needs to recalculate element validity", and the name followed "setNeedsStyleRecalc".  But we need to calculate validity in setNeedsValidityCheck for some reasons.

I don't think we'll have a large refactor for needsValidityCheck, but we should rename it.

[Alexey Proskuryakov]

Comment on attachment 67754 [details]
Patch 2 (resolve redundant setNeedsValidityCheck())

OK. So, do you prefer the old or the new patch yourself? Given that you kept the new one for review, I guess that's it, so I'll mark it r+.

> ta.checkValidity(); // This made an assertion failure.

"This made an assertion fail".

[Kent Tamura]

(In reply to comment #7)
> (From update of attachment 67754 [details])
> OK. So, do you prefer the old or the new patch yourself? Given that you kept the new one for review, I guess that's it, so I'll mark it r+.

Yeah, I prefer the second patch. Thanks for r+.

> > ta.checkValidity(); // This made an assertion failure.
> 
> "This made an assertion fail".

I'll fix it before landing.

[Kent Tamura]

Landed as r68494.