1328894 - location.protocol behaves incorrectly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Anne (:annevk)]

In particular, per HTML navigation should only happen if the new scheme is an HTTP(S) scheme. Tests:

http://w3c-test.org/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

The following test is mostly identical to the above (but does not have the data URL tests), but fails in a rather weird way that needs looking into:

http://w3c-test.org/html/browsers/history/the-location-interface/location-protocol-setter-non-broken-weird.html

Comment 1

[Anne (:annevk)]

(If you set location.protocol to mailto Gecko tries to open an email client, despite that never resulting in a working email address URL...)

Comment 2

[arthur200126]

Spec note: location.protocol's setter would invoke the "basic url parser"[1] defined in URL spec[2]. The parser terminates when the boolean "specialness" of the old and new schemes don't match. Special schemes[3] include ftp, file, gopher, http, https, ws, and wss.


[1]: https://wayback.archive.org/web/20170104144240/https://html.spec.whatwg.org/multipage/semantics.html#reinitialise-url
[2]: https://wayback.archive.org/web/20170103034408/https://url.spec.whatwg.org/#scheme-state
[3]: https://wayback.archive.org/web/20170103034408/https://url.spec.whatwg.org/#urls

(WHATWG site seems down, using archive)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Don't navigate when location.protocol is set to anything other than http or https

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Strip ':' and anything following it from the string passed to the location.protocol setter

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Throw SyntaxError from Location::SetProtocol on URI parse failures

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8834506 [details] [diff] [review]
part 2.  Strip ':' and anything following it from the string passed to the location.protocol setter

Review of attachment 8834506 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/browsers/history/the-location-interface/location-protocol-setter-with-colon.sub.html
@@ +19,5 @@
> +  /* See comments below for why we don't test the https case */
> +  /*
> +    "https-and-gunk": { test: async_test("Set location.protocol to https:gunk"),
> +                      result: "https:" },
> +  */

See comment below.  I don't think we should leave code commented out in a test to be upstreamed.

@@ +56,5 @@
> +    var loc = document.getElementById("https-and-gunk").contentWindow.location;
> +    loc.port = {{ports[https][0]}};
> +    loc.protocol = "https:gunk";
> +  });
> +  */

I'm not sure its a good idea to leave commented out code in a test that will be upstreamed.  I think the explanatory comment is enough.

nit: Also please use // comments for the explanation.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

We still fail the data: cases in these test files?

  location-protocol-setter-non-broken-weird.html.ini
  location-protocol-setter-non-broken.html.ini

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I'm not sure its a good idea to leave commented out code in a test that will be upstreamed.

So the issue is that I want to explain what I would _like_ to test clearly, in case someone figures out a way to do it, along with why I just don't test it.

I guess I can do that as prose instead of code.

> We still fail the data: cases in these test files?

Yes, because we don't implement https://url.spec.whatwg.org/#scheme-state step 2 substep 1.  I intensely dislike the "special scheme" whitelist in the URL spec there, because it's highly hostile to extensions adding support for new protocols.  So if we were to do that, we would need to put some thought into how to do it properly and how it should map onto the set of protocol handlers we have.

Comment 9

[Anne (:annevk)]

It's hostile to our current system for extensions, which allows them to directly parse the input. It's not hostile to a system which hands an extension a parsed URL that the extension then can further process. (Which seems like a better solution given general-purpose URL parsers.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

No, it's hostile to both, because the behavior of |location.protocol = something| becomes very unpredictable: it depends on whether the "something" is a "special scheme" and on whether your current scheme is "special".

Put another way, it makes it impossible to add a new scheme with semantics "like HTTPS except for the on-the-wire protocol" or whatever.

Comment 11

[Anne (:annevk)]

No, test://something/ parses very similarly to https://something/. There are differences (and you cannot switch using location.protocol), but that's simply not true as a blanket statement.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> and you cannot switch using location.protocol

Right, that _is_ the topic under discussion here.  I'm not saying you can't get _similar_ semantics.  I'm saying you can't get _identical_ ones.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d229f56d3ba
part 1.  Don't navigate when location.protocol is set to anything other than http or https.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebd23243cdf
part 2.  Strip ':' and anything following it from the string passed to the location.protocol setter. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e96f6befce
part 3.  Throw SyntaxError from Location::SetProtocol on URI parse failures.  r=bkelly

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d229f56d3ba
https://hg.mozilla.org/mozilla-central/rev/7ebd23243cdf
https://hg.mozilla.org/mozilla-central/rev/28e96f6befce