1850954 - Cant change URL.protocol since v117

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Han]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/117.0

Steps to reproduce:

//Javascript to change the protocol of an URL instance
const url = new URL("https://www.anypage.com?query=something")
url.protocol = "nlp:"
console.log(url.href)

Actual results:

//protocol of the URL object stays on "http" or "https" and does not change.
url.href equals "https://www.anypage.com?query=something"

Expected results:

//this worked before upgrading to Firefox version 117
//protocol of the URL object changes to "nlp:"
url.href equals "nlp://www.anypage.com?query=something"

Comment 1

[Tom S [:evilpie]]

The new behavior is the correct as defined by the specification, but maybe not web compatible. Did this break your website?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking: HTTP' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:twisniewski, since you are the author of the regressor, bug 1347459, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Han]

(In reply to Tom S [:evilpie] from comment #1)

The new behavior is the correct as defined by the specification, but maybe not web compatible. Did this break your website?

Yes it breaks some features of my website. The custom protocols are needed to launch applications. Since it works for other browsers and also worked for Firefox in previous version aswell, I need to implement temporarily a workaround for Firefox users.

Comment 5

[Thomas Wisniewski [:twisniewski]]

Alright, then since bug 1347459 is causing webcompat issues, I think it would be best to revert it and file a spec bug.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Chrome also fixed this in https://bugs.chromium.org/p/chromium/issues/detail?id=1416018 (Chrome 118).
See https://chromium-review.googlesource.com/c/chromium/src/+/4744799
Safari hasn't yet fixed the issue, so we were the first to ship this.

The question is whether we want to back this out and/or put it behind a pref until the other browsers ship it too.
Thomas, do you want to take the lead on this?

Comment 7

[Thomas Wisniewski [:twisniewski]]

I'm working on a backout patch now, but I don't have too much free time to push for a spec update right now. Do you think you or someone else could help out with that?

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1347459

Comment 9

[Ryan VanderMeulen [:RyanVM]]

So Google's going to be shipping this same change in ~5 weeks?

Comment 10

[Thomas Wisniewski [:twisniewski]]

Good point, Ryan. If Google is about to ship this, then maybe it's worth thinking about this a bit more before doing any backouts, given that this is the only report we've received about breaking in two months.

Han, are the changes you need to make drastic?

Comment 11

[Han]

The change itself is quite simple I guess. Currently I am not sure which is the best way to solve this (fast and secure), I go the ugly way with string replace (regex) or I build a new URL object with something like

//js
new Url("customprotocol://" + oldUrl.host + oldUrl.pathname + oldUrl.search + oldUrl.hash)

which is kinda reinventing the wheel to me, guess url.href is doing this already.

I am not sure how much you guys are affected by the fact, that my personal challange comes with rolling out the hotfix to more than 1k clients and that I am bound to strict release cyclies and release time frames.
But I understand, since its a wave which will hit me anyway sooner or later with Google Chrome, MS Edge (a.k.a mini Chrome), Safari and so on, I need to come up with something.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

I left a comment on this standard issue: https://github.com/whatwg/url/issues/674#issuecomment-1701304722

I think it's best to backout the regressor for now in order to avoid webcompat causing issues for our users.
We can probably land it back in nightly with a pref toggle, depending on how the standard discussions go.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

We can take it in the mid-cycle dot release maybe. Might be worth letting this percolate for a bit to see what other reports we get. Given that timing, would it make more sense to go with the pref route rather than backing out?

Comment 14

[Thomas Wisniewski [:twisniewski]]

Valentin and I briefly chatted a little earlier about this, and we've decided to back this out for now while we await a reply from Google. We may re-land this behind a pref soon, but for now I'll land a backout patch here unless the try-run shows any worrying signs: https://treeherder.mozilla.org/jobs?repo=try&revision=38b300afe372cff51f7238f1dc4eee414908d9de

Comment 15

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1850954 - Back out URL protocol spec-compliant restrictions added in bug 1347459 for causing webcompat issues; r?valentin

Comment 16

[Karl Dubost💡 :karlcow]

For WebKit, Bug 229427 - URL.protocol setter should forbid switching between special and non-special schemes

Comment 17

[Han]

(In reply to Thomas Wisniewski [:twisniewski] from comment #10)

Good point, Ryan. If Google is about to ship this, then maybe it's worth thinking about this a bit more before doing any backouts, given that this is the only report we've received about breaking in two months.

Han, are the changes you need to make drastic?

For the sake of correctness I wanted to mention that Firefox v117 was released on August 29th, so two days before this report. I read the second time "in two months" (here and also in the Webkit bug entry). I'm a little confused about the calculation, are you referring to beta releases?

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox117 (release), tracked for firefox118 (beta) and tracked for firefox119 (nightly). However, the bug still has low severity.

:ghess, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fcbbb10c2e5
Back out URL protocol spec-compliant restrictions added in bug 1347459 for causing webcompat issues; r=valentin,necko-reviewers,kershaw

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8fcbbb10c2e5

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Chromium 118 goes to Stable Release on Tue, Oct 10, 2023.
According to Chrome devs, they don't intend to revert the change.
The only question is whether we want to uplift the backout to the dot release, or be the first to ship this change and take the webcompat hit (if there is any) for a little over a month.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:twisniewski, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

If we want to get this into next week's 117.0.1 release, this needs Beta and Release approval requests ASAP.

Comment 25

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9351097 [details]
Bug 1850954 - Back out URL protocol spec-compliant restrictions added in bug 1347459 for causing webcompat issues; r?valentin

Beta/Release Uplift Approval Request

• User impact if declined: Potential for sites breaking. Although changing URL protocols isn't that common, it could definitely break behaviour if a website isn't expecting it. We should back out this change, and land it at a later time (after Chrome does so also, and with a pref).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Reverts to previous behaviour.
• String changes made/needed:
• Is Android affected?: Yes

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9351097 [details]
Bug 1850954 - Back out URL protocol spec-compliant restrictions added in bug 1347459 for causing webcompat issues; r?valentin

Approved for 118.0b7, thanks.

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/da9c89fab6b6

Comment 28

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/da9c89fab6b6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9351097 [details]
Bug 1850954 - Back out URL protocol spec-compliant restrictions added in bug 1347459 for causing webcompat issues; r?valentin

Approved for 117.0.1.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/9470de289d8d

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Added to the 117.0.1 release notes:

Temporarily reverted an intentional behavior change preventing Javascript from changing URL.protocol (bug 1850954). NOTE: This change is expected to ship in a later Firefox release alongside other web browsers and sites are encouraged to find alternate ways to change the protocol if needed.