Disallow mutating the internals of TextInput #20051

jonleighton · 2018-02-14T21:21:51Z

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

This change is

highfive · 2018-02-14T21:21:54Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/textcontrol.rs, components/script/textinput.rs
@fitzgen: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/textcontrol.rs, components/script/textinput.rs
@KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/textcontrol.rs, components/script/textinput.rs

highfive · 2018-02-14T21:21:56Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

jonleighton · 2018-02-14T21:27:36Z

components/script/dom/htmlinputelement.rs

+
+                if *textinput.single_line_content() != value {
+                    // Steps 1-2
+                    textinput.set_content(value, update_text_cursor);


Bit of a shame to reorder this, but it does prevent us setting the value, then getting it for the sanitize step and then setting it again after sanitization... and the outcome is the same.

jdm · 2018-02-15T13:56:27Z

components/script/dom/htmlinputelement.rs

                // Step 4.
                if update_text_cursor {
-                    self.sanitize_value();
+                    self.sanitize_value(&mut value);


No need to check update_text_cursor any more.

Done (although that's not really specific to this change, since update_text_cursor is/was always true here)

The TextInput::assert_ok_selection() method is meant to ensure that we are not getting into a state where a selection refers to a location in the control's contents which doesn't exist. However, before this change we could have a situation where the internals of the TextInput are changed by another part of the code, without using its public API. This could lead to us having an invalid selection. I did manage to trigger such a situation (see the test added in this commit) although it is quite contrived. There may be others that I didn't think of, and it's also possible that future changes could introduce new cases. (Including ones which trigger panics, if indexing is used on the assumption that the selection indices are always valid.) The current HTML specification doesn't explicitly say that selectionStart/End must remain within the length of the content, but that does seems to be the consensus reached in a discussion of this: whatwg/html#2424 The test case I've added here is currently undefined in the spec which is why I've added it in tests/wpt/mozilla.

jdm · 2018-02-22T16:13:16Z

@bors-servo: r+

bors-servo · 2018-02-22T16:13:17Z

📌 Commit 32f7812 has been approved by jdm

bors-servo · 2018-02-22T16:47:43Z

⌛ Testing commit 32f7812 with merge f6463c8...

Disallow mutating the internals of TextInput The TextInput::assert_ok_selection() method is meant to ensure that we are not getting into a state where a selection refers to a location in the control's contents which doesn't exist. However, before this change we could have a situation where the internals of the TextInput are changed by another part of the code, without using its public API. This could lead to us having an invalid selection. I did manage to trigger such a situation (see the test added in this commit) although it is quite contrived. There may be others that I didn't think of, and it's also possible that future changes could introduce new cases. (Including ones which trigger panics, if indexing is used on the assumption that the selection indices are always valid.) The current HTML specification doesn't explicitly say that selectionStart/End must remain within the length of the content, but that does seems to be the consensus reached in a discussion of this: whatwg/html#2424 The test case I've added here is currently undefined in the spec which is why I've added it in tests/wpt/mozilla.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20051)

bors-servo · 2018-02-22T17:41:12Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing f6463c8 to master...

highfive assigned wafflespeanut Feb 14, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 14, 2018

jonleighton force-pushed the prevent-invalid-selections branch from 6d31431 to 66a0211 Compare February 14, 2018 21:24

jonleighton commented Feb 14, 2018

View reviewed changes

jdm assigned jdm and unassigned wafflespeanut Feb 14, 2018

jonleighton force-pushed the prevent-invalid-selections branch from 66a0211 to 1421443 Compare February 15, 2018 08:10

jdm requested changes Feb 15, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 15, 2018

jonleighton force-pushed the prevent-invalid-selections branch from 1421443 to cdca352 Compare February 15, 2018 20:06

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 15, 2018

jonleighton force-pushed the prevent-invalid-selections branch from cdca352 to 0d11f19 Compare February 15, 2018 20:14

jonleighton force-pushed the prevent-invalid-selections branch from 0d11f19 to 32f7812 Compare February 16, 2018 10:45

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 22, 2018

jdm mentioned this pull request Feb 22, 2018

Revert previously merged code, better implementation for Default selectionStart and selectionEnd #20089

Merged

4 tasks

bors-servo merged commit 32f7812 into servo:master Feb 22, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 22, 2018

bors-servo mentioned this pull request Feb 22, 2018

More readable code and major fixes for form reset #20069

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow mutating the internals of TextInput #20051

Disallow mutating the internals of TextInput #20051

jonleighton commented Feb 14, 2018 •

edited by SimonSapin

highfive commented Feb 14, 2018

highfive commented Feb 14, 2018

jonleighton Feb 14, 2018

jdm Feb 15, 2018

jonleighton Feb 15, 2018

jdm commented Feb 22, 2018

bors-servo commented Feb 22, 2018

bors-servo commented Feb 22, 2018

bors-servo commented Feb 22, 2018

Disallow mutating the internals of TextInput #20051

Disallow mutating the internals of TextInput #20051

Conversation

jonleighton commented Feb 14, 2018 • edited by SimonSapin

highfive commented Feb 14, 2018

highfive commented Feb 14, 2018

jonleighton Feb 14, 2018

Choose a reason for hiding this comment

jdm Feb 15, 2018

Choose a reason for hiding this comment

jonleighton Feb 15, 2018

Choose a reason for hiding this comment

jdm commented Feb 22, 2018

bors-servo commented Feb 22, 2018

bors-servo commented Feb 22, 2018

bors-servo commented Feb 22, 2018

jonleighton commented Feb 14, 2018 •

edited by SimonSapin