1401657 - TextControlState::SetValue can trigger an incorrect non-notifying state update

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

See attached testcase, which is red.  Copying bug 1386530 comment 6:

That's because HTMLInputElement::HandleTypeChange also calls HTMLInputElement::SetValueInternal (because we're switching to VALUE_MODE_VALUE), which calls nsTextEditorState::SetValue, which calls nsTextEditorState::SetValue, which calls HTMLInputElement::OnValueChanged, which calls HTMLInputElement::UpdateAllValidityStates.  And in this case aNotify is false because nsTextEditorState::SetValue has a null mRootNode, so doesn't bother to notify.

And even if we didn't call UpdateAllValidityStates from OnValueChanged, we'd still call UpdateState(aNotify) (because we have a placeholder and are now a type to which placeholder applies) which would fail in the same way.

Ehsan, is there a reason we skip notifying from nsTextEditorState::SetValue if !mRootNode?

Comment 1

[(no longer active)]

Hmm...  I don't really remember too be honest.  I tracked this all the way back to attachment 447457 [details] [diff] [review], and I think that was a broken fix to a broken code.

Looking at the setup here, HTMLInputElement::HandleTypeChange() has an aNotify argument.  I think we should be piping that same argument all the way down the call chain described in comment 0, instead of using !!mRootNode, which basically means whether the text control has received a frame, which is why the test case is broken (because the input control is display:none).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, right, the testcase is broken because I purposefully wrote it to hit the !mRootNode case.  My original testcase involved a reframe happening at the same time as some other changes.

That said, piping aNotify through SetValue* is probably doable...  I don't have any better ideas for now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The callstack now passes through TextControlState::SetValue to get to OnValueChanged, but otherwise I think it's about the same issue. The testcase still fails, though now it's racy; probably doing an explicit flush before the mutation would make it fail reliably.

Emilio, do you think you might be interested in poking at this?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yes, bug 1621273 is related and is already in my queue to look at ASAP.

I think bug 1551196 is just a dupe of this... :-)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1401657 - Make editor value changes always notify. r=masayuki

Not doing so is unsound in some cases, see the two referenced bugs.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b0dee0df8c9
Make editor value changes always notify. r=masayuki

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5c4cdbb59c9
Remove ini file since selector-placeholder-shown-type-change-001.html is passing a=test-only CLOSED TREE

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6b0dee0df8c9
https://hg.mozilla.org/mozilla-central/rev/b5c4cdbb59c9