Investigate failure of URL parsing tests

[gterzian]

Follow up from #26231

The PR marks some tests as FAIL, see https://github.com/servo/servo/pull/26231/files#diff-6c940171cdc241cde1caf796556840caR132

Actually that file contained a bunch of existing test failures already, related to things other than the use of location, and those could be investigated as well.

For discussion, see #26231 (comment)

[gterzian]

The spec is mainly at https://html.spec.whatwg.org/multipage/#parse-a-url

Which is used in various places in the /url/failure.html test via the use of other APIs, for example https://html.spec.whatwg.org/multipage/#the-location-interface:parse-a-url or https://html.spec.whatwg.org/multipage/#apis-for-creating-and-navigating-browsing-contexts-by-name:parse-a-url

[gterzian]

Also, the test is called failure.html, so we're not investigating why it fails, we're investigating why it fails to fail!

[gterzian]

Code would be in various places, such as:

• https://github.com/servo/servo/blob/master/components/script/dom/location.rs

• servo/components/script/dom/windowproxy.rs

  Line 455 in 412bc1c

  Err(_) => return None, // TODO: throw a "SyntaxError" DOMException.

• other places where the equivalent of url.join is called.

[utsavoza]

@gterzian I see that the return type of window.open() is currently Option<DomRoot<WindowProxy>> as per the spec and Window.webidl.

However, in step 13 of the window open steps we are expected to throw a "SyntaxError" DomException when the parse URL algorithm fails (as listed above). So if the operation is fallible, shouldn't the IDL specify it as such?

[Throws] WindowProxy? open(optional USVString url = "", optional DOMString target = "_blank", optional [LegacyNullToEmptyString] DOMString features = "");

As far as I can tell, in Gecko we do consider the operation as fallible.

[gterzian]

Yes I think that's correct, good catch, so that would then indeed be a change required to implement the URL parsing changes.

[utsavoza]

So, I did update the window.open() method to return a fallible result and it seems we are able to pass quite a few url failure tests for window.open(). I am not sure if I should raise a PR that partially resolves the issue.

Also, I think the urls for which we had to mark some tests as FAIL for the location interface also got parsed correctly here for window.open(), though I haven't gone through the url.join implementation exhaustively yet.

[gterzian]

I am not sure if I should raise a PR that partially resolves the issue.

yes I think a PR just for the window.open part would be great!

Let me know what you find out about the other tests...

[utsavoza]

@gterzian We'll need to re-open the issue as PR #26410 only partially resolves the issue.

[gterzian]

Ok, thanks! I didn't see that PR before it had already merged! :)

[utsavoza]

After digging a little bit, I was able narrow down the issue to two things:

1. Currently, servo is pulling in url crate version 2.1.0 instead of 2.1.1 and based on this PR itself (and updating the dependency locally to verify it), a lot of currently failing tests are resolved.

2. Based on the spec, I think our implementation of some location setters (specifically href) incorrectly fetches the document URL instead of base URL for the entry settings object.
   

   servo/components/script/dom/location.rs

   Lines 254 to 256 in 6aec2c8

   	// Step 2: Parse the given value relative to the entry settings object.
   	// If that failed, throw a TypeError exception.
   	let base_url = self.entry_document().url();

   And the document's implementation of base_url (specifically fallback_base_url) is currently incomplete:
   

   servo/components/script/dom/document.rs

   Lines 573 to 579 in 6aec2c8

   	// https://html.spec.whatwg.org/multipage/#fallback-base-url
   	pub fn fallback_base_url(&self) -> ServoUrl {
   	// Step 1: iframe srcdoc (#4767).
   	// Step 2: about:blank with a creator browsing context.
   	// Step 3.
   	self.url()
   	}

@gterzian I wonder if I should address these in two separate PRs?

[jdm]

Addressing those separately makes sense to me. The fix for the fallback base url would also help in #24944 (comment) which has some additional information; it's going to require passing some information around about the browsing context creator when loading iframes so documents can store it appropriately.

[utsavoza]

Thanks, since I have tried updating the dependency locally, I can raise a PR for it right away (though I haven't run all the wpt tests), updating the fallback_url might need some more work on my end.

[utsavoza]

I'd like to reiterate the observations in #26231 (comment). As per the README:

In addition to testing that parsing input against base gives the result, a test harness for the URL constructor (or similar APIs) should additionally test the following pattern: if failure is true, parsing about:blank against input must give failure. This tests that the logic for converting base URLs into strings properly fails the whole parsing algorithm if the base URL cannot be parsed.

For all the tests that are expected to fail, the input must be parsed against about:blank as the base url. However, during the test execution, the base url that get's picked up is http://web-platform.test:8000/url/failure.html. Even though this eliminates several bad urls, there are still some edge cases that gets parsed correctly (according to the spec) against a valid base url but fails against about:blank.

Edit: It seems Firefox also marks these tests as failure.