Closed
Bug 1401657
Opened 7 years ago
Closed 5 years ago
TextControlState::SetValue can trigger an incorrect non-notifying state update
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla76
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: emilio)
References
Details
(Whiteboard: [wpt-triaged])
Attachments
(2 files)
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?
Flags: needinfo?(ehsan)
Updated•7 years ago
|
Priority: -- → P2
Comment 1•7 years ago
|
||
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).
Flags: needinfo?(ehsan)
Reporter | ||
Comment 2•7 years ago
|
||
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.
Updated•5 years ago
|
Whiteboard: [wpt-triaged]
Reporter | ||
Comment 3•5 years ago
|
||
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?
Flags: needinfo?(emilio)
Reporter | ||
Updated•5 years ago
|
Summary: nsTextEditorState::SetValue can trigger an incorrect non-notifying state update → TextControlState::SetValue can trigger an incorrect non-notifying state update
Assignee | ||
Comment 4•5 years ago
|
||
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... :-)
Assignee | ||
Comment 5•5 years ago
|
||
Not doing so is unsound in some cases, see the two referenced bugs.
Updated•5 years ago
|
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b0dee0df8c9
Make editor value changes always notify. r=masayuki
Pushed by btara@mozilla.com:
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6b0dee0df8c9
https://hg.mozilla.org/mozilla-central/rev/b5c4cdbb59c9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox76:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
You need to log in
before you can comment on or make changes to this bug.
Description
•