Closed Bug 1328894 Opened 4 years ago Closed 4 years ago

location.protocol behaves incorrectly

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: annevk, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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
(If you set location.protocol to mailto Gecko tries to open an email client, despite that never resulting in a working email address URL...)
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)
Priority: -- → P2
Blocks: 1336191
Blocks: 726779
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8834505 - Flags: review?(bkelly) → review+
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.
Attachment #8834506 - Flags: review?(bkelly) → review+
Attachment #8834508 - Flags: review?(bkelly) → review+
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
> 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.
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.)
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.
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.
> 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.
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1d229f56d3ba
https://hg.mozilla.org/mozilla-central/rev/7ebd23243cdf
https://hg.mozilla.org/mozilla-central/rev/28e96f6befce
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1328843
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.