Implement various checks on the Location setters #26231

utsavoza · 2020-04-20T16:09:24Z

Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement various checks on the Location setters #23522
There are tests for these changes

highfive · 2020-04-20T16:09:32Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/location.rs
@KiChjang: components/script/dom/location.rs

gterzian

Great work! Thanks for figuring out all those details. I think there are a few steps missing(although you could open an issue and leave that as a follow-up if there is a reason for that), and I have a small suggestion re the set_url_component method.

Also, could you please squash into one commit? I think while it's a good idea to separate unrelated work into commits, in this case it is actually related and consolidating the work into a commit would appear clearer.

gterzian · 2020-04-21T02:14:22Z

components/script/dom/location.rs

+            self.check_same_origin_domain()?;
+            // Step 3: Let copyURL be a copy of this Location object's url.
+            let copy_url = self.get_url();
+            // Step 4: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.


Are we doing step 5 currently? If not, I think we can do it using as_mut_url().set_path.

Oh, I assumed we were executing those steps as a part of url::quirks::set_path through UrlHelper::SetPathname, but those steps seem to correspond to dom url spec. I'll add those steps here.

gterzian · 2020-04-21T02:15:42Z

components/script/dom/location.rs

+            // Step 3: Let copyURL be a copy of this Location object's url.
+            let copy_url = self.get_url();
+            // Step 4: If copyURL cannot have a username/password/port, then return.
+            // https://url.spec.whatwg.org/#cannot-have-a-username-password-port


Similar re step 5, which could be done using set_port.

There's one caveat though when setting port directly via as_mut_url().set_port - we'll have to manually verify and parse the input string to Option<u16>. However, we can use url::quirks::set_port to check and set the port here instead of UrlHelper::SetPort. Please let me know if I'm overlooking any better way to solve this.

Sounds good!

gterzian · 2020-04-21T02:21:11Z

components/script/dom/location.rs

+            // same origin-domain with the entry settings object's origin, then
+            // throw a "SecurityError" DOMException.
+            self.check_same_origin_domain()?;
+            self.set_url_component(value, UrlHelper::SetSearch);


I think we're missing step 4 and 5 here? It looks like set_query can be used for step 4, and step 5 would be those additional manipulation of input and copyUrl before calling set_url_component.

Again, I presumed those steps as a part of UrlHelper::SetSearch. I'll add the required steps here.

gterzian · 2020-04-21T02:22:11Z

components/script/dom/location.rs

+            let copy_url = self.get_url();
+            // Step 4: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
+            if !copy_url.cannot_be_a_base() {
+                self.set_url_component(value, UrlHelper::SetHostname);


How about we add a copy_url argument to set_url_component to avoid all those double calls to get_url(for algorithms that need the url before calling set_url_component)?

We could also add a documentation to set_url_component to note that it is supposed to match the steps related to https://url.spec.whatwg.org/#concept-basic-url-parser and https://html.spec.whatwg.org/multipage/history.html#location-object-setter-navigate, and then at the call site we could also comment to which local steps in the algorithm the call corresponds.

I think we can actually remove the method completely - the only place we might actually need this method after adding checks would be for SetPort method probably (as we might not directly setting port to the url there). For the rest of the setters, we can directly call the navigate method with the appropriate arguments.

utsavoza · 2020-04-21T13:26:56Z

Also, could you please squash into one commit? I think while it's a good idea to separate unrelated work into commits, in this case it is actually related and consolidating the work into a commit would appear clearer.

Agreed. I'll squash into one commit once the changes are approved.

gterzian

Looks like a big improvement in the correctness of the implementation! Just one nit left, and then after you squash I'll approve it(after which we might still have to deal with some surprise failures on the test run).

gterzian · 2020-04-21T15:40:08Z

components/script/dom/location.rs

+            let mut copy_url = self.get_url();
+            // Step 4: Let input be the given value with a single leading "#" removed, if any.
+            // Step 5: Set copyURL's fragment to the empty string.
+            // Step 6:Basic URL parse input, with copyURL as url and fragment state as


nit: space missing.

gterzian · 2020-04-21T16:18:14Z

components/script/dom/location.rs

+            // Step 3: Let copyURL be a copy of this Location object's url.
+            let copy_url = self.get_url();
+            // Step 4: If copyURL cannot have a username/password/port, then return.
+            // https://url.spec.whatwg.org/#cannot-have-a-username-password-port


Sounds good!

utsavoza · 2020-04-21T16:35:52Z

Looks like a big improvement in the correctness of the implementation!

@gterzian Thanks! I have made the suggested change, and squashed all commits into one.

Just one nit left, and then after you squash I'll approve it(after which we might still have to deal with some surprise failures on the test run).

Actually, I ran the wpt tests for the location interface locally. Apart from that how do I verify if we aren't breaking anything else?

gterzian · 2020-04-22T03:28:04Z

Actually, I ran the wpt tests for the location interface locally. Apart from that how do I verify if we aren't breaking anything else?

We have to basically run the whole suite and see, there can also be tests that unexpectedly start passing...

gterzian · 2020-04-22T03:28:23Z

Thanks for the awesome work!

@bors-servo r+

bors-servo · 2020-04-22T03:28:24Z

📌 Commit 79cffb3 has been approved by gterzian

bors-servo · 2020-04-22T04:00:28Z

⌛ Testing commit 79cffb3 with merge 4a364f5...

@uxioandrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes

bors-servo · 2020-04-22T04:23:40Z

💔 Test failed - status-taskcluster

gterzian · 2020-04-22T04:43:12Z

This one seems relevant:

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html:
  │ FAIL [expected PASS] Set location from a function called from a parent
  │   → assert_equals: expected "http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/source/support/location-set.html" but got "http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html"
  │ 
  │ onload</fr.onload<@http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html:15:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2001:25
  └ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2042:32

This one does not seem relevant(I'm not sure what intermittent issue it would match by the way):

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /_mozilla/mozilla/scroll_top_null_target.html:
  │ FAIL [expected PASS] scroll_top_null_target
  │   → assert_equals: Target should be null! expected null but got Element node <a id="test"></a>
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/scroll_top_null_target.html:18:23
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2001:25
  │ test@http://web-platform.test:8000/resources/testharness.js:548:30
  └ @http://web-platform.test:8000/_mozilla/mozilla/scroll_top_null_target.html:13:10

gterzian · 2020-04-22T04:45:11Z

Ok the first one might actually be a new intermittent, perhaps you fixed something that makes it show up, and since it seems related to navigation, it's probably best done in a follow up. So please take a look, and if it doesn't appear to point out an issue with this PR, just file an issue for it(if there isn't one already) and add the "intermittent" label to it.

jdm · 2020-04-22T04:50:52Z

@bors-servo try=wpt

gterzian · 2020-04-22T04:51:54Z

this one seems defenitely relevant:

  ▶ Unexpected subtest result in /url/failure.html:
  │ FAIL [expected PASS] Location's href: http:/:@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:/@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:a:b@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:/a:b@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http::@/www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:@:www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: http:/@:www.example.com should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: a should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: a/ should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ FAIL [expected PASS] Location's href: a// should throw
  │   → assert_throws_js: function "() => self[0].location = test.input" did not throw
  │ 
  │ runTests/<@http://web-platform.test:8000/url/failure.html:41:23
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2001:25
  │ test@http://web-platform.test:8000/resources/testharness.js:548:30
  └ runTests@http://web-platform.test:8000/url/failure.html:40:10

gterzian · 2020-04-22T05:01:58Z

components/script/dom/location.rs

+                Err(e) => return Err(Error::Type(format!("Couldn't parse URL: {}", e))),
+            };
+            // Step 3: Location-object-setter navigate to the resulting URL record.
+            let referrer = Referrer::ReferrerUrl(base_url.clone());


Ok I think this is the problem with /html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html, the referrer should be not from the entry document but from self.window.Document, at least that is what I make from the test failure(so it's not a new intermittent and not actually related much to navigation).

The test has a page call a method on an iframe, so the page is the "entry document" whose url would be the base url, however the referrer should still be url of the iframe itself.

Ahh, right. The referrer url shouldn't be from entry document here - I think let referrer = Referrer::ReferrerUrl(self.get_url()); should fix this.

utsavoza · 2020-04-22T14:10:55Z

I was able to find six relevant test failures:

/url/failure.html
/html/browsers/browsing-the-web/navigating-across-documents/005.html
/html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html
/html/browsers/browsing-the-web/navigating-across-documents/empty_fragment.html
/html/browsers/browsing-the-web/unloading-documents/beforeunload-synchronous.html - [UNEXPECTED PASS]
/fullscreen/api/document-exit-fullscreen-active-document.html - [UNEXPECTED PASS]

and one NOT known-intermittent (as pointed out by @gterzian) from the logs:

./mach filter-intermittents wpt-jsonsummary.log --log-intermittents intermittents.log --log-filteredsummary filtered-wpt-errorsummary.log --tracker-api default --reporter-api default
1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /_mozilla/mozilla/scroll_top_null_target.html:
  │ FAIL [expected PASS] scroll_top_null_target
  │   → assert_equals: Target should be null! expected null but got Element node <a id="test"></a>
  │
  │ @http://web-platform.test:8000/_mozilla/mozilla/scroll_top_null_target.html:18:23
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2001:25
  │ test@http://web-platform.test:8000/resources/testharness.js:548:30
  └ @http://web-platform.test:8000/_mozilla/mozilla/scroll_top_null_target.html:13:10

I will try to resolve these and post any updates here.

jdm · 2020-04-22T14:14:59Z

Filed #26269 for the new unrelated intermittent test result.

gterzian · 2020-04-22T16:09:17Z

@utsavoza awesome, sounds like it's worth updating this PR with your work and we'll give the test suite another try...

Implement various checks on the Location setters Set correct referrer url before for href location setter Update wpt-metadata for failing tests Try updating tests/wpt/metadata/url/failure.html.ini

utsavoza · 2020-04-24T04:39:17Z

Done!

gterzian · 2020-04-24T05:22:38Z

@utsavoza Thanks for completing this, it was a lot of additional work in the end!

@bors-servo r+

bors-servo · 2020-04-24T05:22:39Z

📌 Commit b10de77 has been approved by gterzian

bors-servo · 2020-04-24T05:22:43Z

⌛ Testing commit b10de77 with merge bd86ff8...

@uxioandrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes

bors-servo · 2020-04-24T06:04:03Z

💔 Test failed - status-taskcluster

gterzian · 2020-04-24T06:53:05Z

@bors-servo retry

#21067
#26298
#25513
#26299
#25513

@uxioandrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes

bors-servo · 2020-04-24T07:31:32Z

⌛ Testing commit b10de77 with merge 0123fed...

bors-servo · 2020-04-24T08:14:13Z

💔 Test failed - status-taskcluster

CYBAI · 2020-04-24T09:30:15Z

@bors-servo retry

Intermittent crash during shutdown in reftests #25533, Frequent intermittent timeouts in macOS WPT tests #25513

bors-servo · 2020-04-24T09:30:21Z

⌛ Testing commit b10de77 with merge 3078b35...

bors-servo · 2020-04-24T09:49:16Z

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 3078b35 to master...

utsavoza · 2020-04-24T09:51:02Z

@utsavoza Thanks for completing this, it was a lot of additional work in the end!

Hey @gterzian, thanks for your guidance.

highfive assigned SimonSapin Apr 20, 2020

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 20, 2020

gterzian requested changes Apr 21, 2020

View reviewed changes

utsavoza force-pushed the ugo/issue-23522/16-04-2020 branch from d6d739a to 63d311b Compare April 21, 2020 14:04

gterzian requested changes Apr 21, 2020

View reviewed changes

utsavoza force-pushed the ugo/issue-23522/16-04-2020 branch 2 times, most recently from 4b8fc42 to 79cffb3 Compare April 21, 2020 16:48

highfive assigned gterzian and unassigned SimonSapin Apr 22, 2020

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 Apr 22, 2020

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 Apr 22, 2020

gterzian reviewed Apr 22, 2020

View reviewed changes

gterzian approved these changes Apr 24, 2020

View reviewed changes

Implement various checks on the Location setters

b10de77

Implement various checks on the Location setters Set correct referrer url before for href location setter Update wpt-metadata for failing tests Try updating tests/wpt/metadata/url/failure.html.ini

utsavoza force-pushed the ugo/issue-23522/16-04-2020 branch from 9c07854 to b10de77 Compare April 24, 2020 04:38

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-rebase There are merge conflict errors. labels Apr 24, 2020

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 Apr 24, 2020

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

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 Apr 24, 2020

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

bors-servo merged commit 3078b35 into servo:master Apr 24, 2020

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 24, 2020

Implement various checks on the Location setters #26231

Implement various checks on the Location setters #26231

Conversation

utsavoza commented Apr 20, 2020

highfive commented Apr 20, 2020

gterzian left a comment • edited

Choose a reason for hiding this comment

gterzian Apr 21, 2020

Choose a reason for hiding this comment

utsavoza Apr 21, 2020 • edited

Choose a reason for hiding this comment

gterzian Apr 21, 2020

Choose a reason for hiding this comment

utsavoza Apr 21, 2020 • edited

Choose a reason for hiding this comment

gterzian Apr 21, 2020

Choose a reason for hiding this comment

gterzian Apr 21, 2020 • edited

Choose a reason for hiding this comment

utsavoza Apr 21, 2020 • edited

Choose a reason for hiding this comment

gterzian Apr 21, 2020 • edited

Choose a reason for hiding this comment

utsavoza Apr 21, 2020

Choose a reason for hiding this comment

utsavoza commented Apr 21, 2020

gterzian left a comment

Choose a reason for hiding this comment

gterzian Apr 21, 2020

Choose a reason for hiding this comment

gterzian Apr 21, 2020

Choose a reason for hiding this comment

utsavoza commented Apr 21, 2020 • edited

gterzian commented Apr 22, 2020

gterzian commented Apr 22, 2020

bors-servo commented Apr 22, 2020

bors-servo commented Apr 22, 2020

bors-servo commented Apr 22, 2020

gterzian commented Apr 22, 2020 • edited

gterzian commented Apr 22, 2020 • edited

jdm commented Apr 22, 2020

gterzian commented Apr 22, 2020

gterzian Apr 22, 2020 • edited

Choose a reason for hiding this comment

utsavoza Apr 22, 2020

Choose a reason for hiding this comment

utsavoza commented Apr 22, 2020

jdm commented Apr 22, 2020

gterzian commented Apr 22, 2020

utsavoza commented Apr 24, 2020

gterzian commented Apr 24, 2020

bors-servo commented Apr 24, 2020

bors-servo commented Apr 24, 2020

bors-servo commented Apr 24, 2020

gterzian commented Apr 24, 2020 • edited

bors-servo commented Apr 24, 2020

bors-servo commented Apr 24, 2020

CYBAI commented Apr 24, 2020

bors-servo commented Apr 24, 2020

bors-servo commented Apr 24, 2020

utsavoza commented Apr 24, 2020

gterzian left a comment •

edited

utsavoza Apr 21, 2020 •

edited

utsavoza Apr 21, 2020 •

edited

gterzian Apr 21, 2020 •

edited

utsavoza Apr 21, 2020 •

edited

gterzian Apr 21, 2020 •

edited

utsavoza commented Apr 21, 2020 •

edited

gterzian commented Apr 22, 2020 •

edited

gterzian commented Apr 22, 2020 •

edited

gterzian Apr 22, 2020 •

edited

gterzian commented Apr 24, 2020 •

edited