Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace #21978

jimberlage · 2018-10-18T01:47:38Z

This changes DOMString::strip_leading_and_trailing_ascii_whitespace to handle multi-byte unicode characters.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix assertion failed: self.is_char_boundary(new_len) in DOMString::strip_leading_and_trailing_ascii_whitespace #21963 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-10-18T01:47:45Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2018-10-18T01:47:45Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/str.rs
@KiChjang: components/script/dom/bindings/str.rs

highfive · 2018-10-18T01:47:47Z

Warning

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

SimonSapin

Looks good, thanks! Comments inline.

SimonSapin · 2018-10-18T06:42:12Z

components/script/dom/bindings/str.rs

-            Some(idx) => idx + 1,
+        match self.0.rfind(|ref c| !char::is_ascii_whitespace(c)) {
+            Some(idx) => {
+                let last_non_whitespace = self.0.char_indices().find(|pair| pair.0 > idx);


This unnecessarily goes though the whole string from the start to idx. I think something like this would also work and be more efficient. The variable names are also slightly wrong (but already were before this PR).

Some(last_non_whitespace) => { let after_last_non_whitespace = last_non_whitespace + // `unwrap` never panics here because `rfind` found at least one char. self.0[last_non_whitespace..].chars().next().unwrap().len_utf8(); self.0.truncate(after_last_non_whitespace)

Now that I’ve typed all this, I realize this whole match block can be removed and better written as:

self.0.truncate(self.0.trim_end_matches(|c| c.is_ascii_whitespace()).len());

Sorry @SimonSapin for spamming your comment, I'm trying to understand servo I have on (maybe stupid :-)) question. With trim_end_matches a &str is created and the length of it is used. Would it also be possible to use trim_matches and replace the String member of the DOMString with the resulting &str or is the member used also somewhere else?

self.0 = trim_matches(…).to_string() would work, but it makes a new memory allocation and copies the preserved part of the string. This algorithm reuses the existing allocation and, if first_non_whitespace happens to be zero, does not need to do any copying.

SimonSapin · 2018-10-18T06:42:17Z

components/script/dom/bindings/str.rs

+            Some(idx) => {
+                let last_non_whitespace = self.0.char_indices().find(|pair| pair.0 > idx);
+                if last_non_whitespace.is_some() {
+                    self.0.truncate(last_non_whitespace.unwrap().0);


I’d recommend if let Some(x) = y instead of is_some + unwrap, but with the change suggested above this branch is gone.

nox · 2018-10-18T08:16:16Z

@jdm Do we really need a unit test for that? I would rather we don't add more unit tests but will defer to you for the decision on whether to keep them in this PR.

nox · 2018-10-18T08:17:32Z

tests/unit/script/lib.rs

@@ -7,6 +7,7 @@
 #[cfg(test)] extern crate script;
 #[cfg(test)] extern crate servo_url;

+#[cfg(test)] mod dom;


Just make a top-level module named str, no need to follow the module hierarchy of the script crate.

jdm · 2018-10-18T11:56:02Z

I would move the unit test cases into the integration test.

jimberlage

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @SimonSapin, @nox, and @asajeffrey)

components/script/dom/bindings/str.rs, line 187 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Now that I’ve typed all this, I realize this whole match block can be removed and better written as:

self.0.truncate(self.0.trim_end_matches(|c| c.is_ascii_whitespace()).len());
</blockquote></details>

Done.
___
*[components/script/dom/bindings/str.rs, line 189 at r1](https://reviewable.io/reviews/servo/servo/21978#-LP3qt50UcnoV6yAPM36-r1-189:-LP8Z0xXBM97cDBU-oPY:b-896fix) ([raw file](https://github.com/servo/servo/blob/4ade61c35b85e9d122ed87e45d16ce355c9a9a35/components/script/dom/bindings/str.rs#L189)):*
<details><summary><i>Previously, SimonSapin (Simon Sapin) wrote…</i></summary><blockquote>

I’d recommend `if let Some(x) = y` instead of `is_some` + `unwrap`, but with the change suggested above this branch is gone.
</blockquote></details>

Done.
___
*[tests/unit/script/lib.rs, line 10 at r1](https://reviewable.io/reviews/servo/servo/21978#-LP3qt50UcnoV6yAPM37-r1-10:-LP8Z2VPA3VrBDZaFsa1:b-896fix) ([raw file](https://github.com/servo/servo/blob/4ade61c35b85e9d122ed87e45d16ce355c9a9a35/tests/unit/script/lib.rs#L10)):*
<details><summary><i>Previously, nox (Anthony Ramine) wrote…</i></summary><blockquote>

Just make a top-level module named `str`, no need to follow the module hierarchy of the script crate.
</blockquote></details>

Done.


<!-- Sent from Reviewable.io -->

jdm · 2018-10-29T12:24:59Z

This looks great! Please squash the two commits into one, and then it will be ready to merge!

jdm · 2018-11-08T15:27:49Z

@bors-servo r+

bors-servo · 2018-11-08T15:27:50Z

📌 Commit 68228c0 has been approved by jdm

bors-servo · 2018-11-08T19:46:00Z

⌛ Testing commit 68228c0 with merge 0175b2b...

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace  This changes `DOMString::strip_leading_and_trailing_ascii_whitespace` to handle multi-byte unicode characters. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #21963 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21978)

bors-servo · 2018-11-08T20:16:20Z

💔 Test failed - linux-dev

jdm · 2018-11-08T20:17:54Z

Diff in /home/servo/buildbot/slave/linux-dev/build/components/script/dom/bindings/str.rs at line 182:
             return;
         }
 
-        let trailing_whitespace_len = self.0.trim_end_matches(|ref c| char::is_ascii_whitespace(c)).len();
+        let trailing_whitespace_len = self
+            .0
+            .trim_end_matches(|ref c| char::is_ascii_whitespace(c))
+            .len();
         self.0.truncate(trailing_whitespace_len);
         if self.0.is_empty() {
             return;
Run `./mach fmt` to fix the formatting

jdm · 2018-11-16T13:32:47Z

@bors-servo r+

bors-servo · 2018-11-16T13:32:48Z

📌 Commit 1f99bf4 has been approved by jdm

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace  This changes `DOMString::strip_leading_and_trailing_ascii_whitespace` to handle multi-byte unicode characters. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #21963 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21978)

bors-servo · 2018-11-16T16:10:35Z

⌛ Testing commit 1f99bf4 with merge 831f966...

bors-servo · 2018-11-16T17:27:02Z

☀️ Test successful - android, android-mac, android-x86, 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, magicleap, status-taskcluster, windows-msvc-dev
Approved by: jdm
Pushing 831f966 to master...

highfive assigned asajeffrey Oct 18, 2018

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

jimberlage force-pushed the input-value-fix branch from ccb80b4 to 4ade61c Compare October 18, 2018 01:48

SimonSapin reviewed Oct 18, 2018

View reviewed changes

nox reviewed Oct 18, 2018

View reviewed changes

jimberlage commented Oct 18, 2018

View reviewed changes

jdm approved these changes Oct 29, 2018

View reviewed changes

jdm assigned jdm and unassigned asajeffrey Oct 29, 2018

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 29, 2018

jimberlage force-pushed the input-value-fix branch from b00604c to 68228c0 Compare November 8, 2018 15:14

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

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. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 8, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 8, 2018

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace

1f99bf4

jimberlage force-pushed the input-value-fix branch from 68228c0 to 1f99bf4 Compare November 8, 2018 22:17

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 8, 2018

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 Nov 16, 2018

bors-servo merged commit 1f99bf4 into servo:master Nov 16, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace #21978

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace #21978

jimberlage commented Oct 18, 2018 •

edited by SimonSapin

highfive commented Oct 18, 2018

highfive commented Oct 18, 2018

highfive commented Oct 18, 2018

SimonSapin left a comment

SimonSapin Oct 18, 2018

SimonSapin Oct 18, 2018

cannero Oct 19, 2018

SimonSapin Oct 29, 2018

SimonSapin Oct 18, 2018

nox commented Oct 18, 2018

nox Oct 18, 2018

jdm commented Oct 18, 2018

jimberlage left a comment

jdm commented Oct 29, 2018

jdm commented Nov 8, 2018

bors-servo commented Nov 8, 2018

bors-servo commented Nov 8, 2018

bors-servo commented Nov 8, 2018

jdm commented Nov 8, 2018

jdm commented Nov 16, 2018

bors-servo commented Nov 16, 2018

bors-servo commented Nov 16, 2018

bors-servo commented Nov 16, 2018

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace #21978

Fixes panic on DOMString::strip_leading_and_trailing_ascii_whitespace #21978

Conversation

jimberlage commented Oct 18, 2018 • edited by SimonSapin

highfive commented Oct 18, 2018

highfive commented Oct 18, 2018

highfive commented Oct 18, 2018

SimonSapin left a comment

Choose a reason for hiding this comment

SimonSapin Oct 18, 2018

Choose a reason for hiding this comment

SimonSapin Oct 18, 2018

Choose a reason for hiding this comment

cannero Oct 19, 2018

Choose a reason for hiding this comment

SimonSapin Oct 29, 2018

Choose a reason for hiding this comment

SimonSapin Oct 18, 2018

Choose a reason for hiding this comment

nox commented Oct 18, 2018

nox Oct 18, 2018

Choose a reason for hiding this comment

jdm commented Oct 18, 2018

jimberlage left a comment

Choose a reason for hiding this comment

jdm commented Oct 29, 2018

jdm commented Nov 8, 2018

bors-servo commented Nov 8, 2018

bors-servo commented Nov 8, 2018

bors-servo commented Nov 8, 2018

jdm commented Nov 8, 2018

jdm commented Nov 16, 2018

bors-servo commented Nov 16, 2018

bors-servo commented Nov 16, 2018

bors-servo commented Nov 16, 2018

jimberlage commented Oct 18, 2018 •

edited by SimonSapin