Description
URIFixup is not the same in Normal & private browsing mode with keyword.enabled = false

Tested and affects all these Os
Ubuntu 20
Windows 10 64bit
MacOs 10.15

Affected
Firefox nightly 92.0a1 affected
Firefox beta 91 affected
Firefox release 90.0.2 not affected

Preconditions
keyword.enabled = false in about:Config

Steps
-Open a normal browsing window (not private)
-Check in about:Config that keyword.enabled = false
-type "cnn" in urlbar and hit ENTER key > this loads the cnn.com website and you can read all the news.
-Open new PRIVATE WINDOW
-in url bar type "cnn" and press ENTER key >

Expected result
should open cnn website in both normal and private? or should fail opening anything in normal and private?

Actual result
Fails to open a site, i get a white page that says "Hmm. We’re having trouble finding that site."

notes
Im not sure if the normal browsing mode is the one with the bug or the private browsing mode is the one with the bug.
The only thing i know its wrong is that normal browsing mode and private browsing mode have a different result, when they should be equal.

One possibility is something in https://searchfox.org/mozilla-central/source/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm that handles keyword.enabled to generate a heuristic result. For some reason that result may differ. We can check the string sent to UrlbarInput._loadURL

Another possibility may be WebNavigation flags being different so that in a case the docshell does alternate fixup, in the other it doesn't, but I didn't find evidence of that so far.

this is what i found in mozregression:

Bug 1709838: Enable HTTPS-First Mode in PBM Mode in Nightly r=arthuredelstein

Differential Revision: https://phabricator.services.mozilla.com/D114500

build_url: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/BTQwXoIpTiqA6z80Vj_XRQ/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: 74fd8bb17de03fdbdb0586843070e2434a0ef2fc
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=74fd8bb17de03fdbdb0586843070e2434a0ef2fc&tochange=80232a13c04e04132963858feb3f94e6f5607c83

Setting bug 1709838 as the regressing bug based on comment 2

Christoph, looks like this is a regression from bug 1709838, could you please look into this, thank you!

I am going to be out on PTO and can't handle that in a timely manner - 302 to Tomer who is our engineer on https-first. Tomer, can you take a look please?

Sure,
I will look into it.
Thanks for reporting!

Tomer, could you provide a status update on this bug. Could this possibly fixed for Fx93?

Sorry for responding so late,
The Problem is that the input e.g. http://ccn gets upgraded to https because of https-first mode which is enabled in private browsing.
The same error can be reproduced when you in normal browsing mode try to reach for https://cnn .
Nevertheless, https-first should downgrade it. I think the error occurs because https-first isn't receiving a response and the occurring error is registers as an unrelated one (https://searchfox.org/mozilla-central/source/dom/security/nsHTTPSOnlyUtils.cpp#473) which leads https-first to not downgrade to http.

It is an interesting one but might be tricky to fix. I am currently unsure if it is possibly that I complete a fix for Fx93.
But I will give my best to fix it asap :)

Report:

1. We are loading the request : https://searchfox.org/mozilla-central/source/netwerk/ipc/DocumentLoadListener.cpp#2126-2131
2. The request http://cnn gets upgraded by https-first to https://cnn.
3. We get an Error NS_ERROR_UNKNOWN_HOST.
4. We enter attemptURIFixup : https://searchfox.org/mozilla-central/source/netwerk/ipc/DocumentLoadListener.cpp#2037
5. Creating a new URI but again the spec is https://cnn . We enter here (https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6082)
6. Then we are trying to load the new URI and it fails.

Probably it would be worth, to check for https_first and https in step 5. If both appear on that step then to change to http. But I am unsure, and out on PTO the upcoming two weeks.

I am sorry that I didn't make it for F93.
But I didn't wanted to take the risk that it is somehow failing and I am on PTO.

Christoph, what's the status here?

(In reply to Marco Castelluccio [:marco] from comment #11)

Christoph, what's the status here?

Tomer is on it - we'll get it fixed within this cycle. If you think it's more urgent, then please ping me again on slack.

Thank you Tomer for pinging me on Slack about this issue.
Gijs, I'd love to get your opinion here.
As we previously discussed, alternate fixup has its issues, that are discussed in bug 1679556, and long term we may want to remove it.
This bug is mostly around the fact alternate fixup only works for http: (https://searchfox.org/mozilla-central/source/docshell/base/URIFixup.jsm#751), and I don't remember the reasons behind that, do you? Was it to avoid sending auth to the wrong target?
This bug could be fixed as easy as changing that code to accept https but:

1. I suspect we have security concerns
2. enlarging the reach of a feature we'd want to remove is a no-go

If https-only flags the url as "modified" somewhere, we could pass that flag to URIFixup and let it bypass the http check only in that case.
What I wonder though if in general it's worth fixing the bug, this is an edge case that is basically disabling alternate fixup in PB windows, that is something ideally we'd like to do everywhere... We're just not ready to do it everywhere. Could doing it in PB become a good first step in that direction though? It sounds like in PB the user may actually not want us to transform urls in general.

(In reply to Marco Bonardo [:mak] from comment #14)

Thank you Tomer for pinging me on Slack about this issue.
Gijs, I'd love to get your opinion here.
As we previously discussed, alternate fixup has its issues, that are discussed in bug 1679556, and long term we may want to remove it.

I think we should do one of the following:

1. disable alternate fixup by default
2. change the pref name to an int pref and migrate people. The values should be something like (0) off, (1) auto (default), (2) always on, and for (1) we determine whether to alternate fixup based on whether keyword.enabled is false.
3. add data collection to determine how frequently alternate fixup is used, and as metadata on the event telemetry, check if keyword.enabled is true or false.
4. like (1) but add convenient links for foo.com and/or "search [engine] for 'foo'" on the error page.

(2) is less risky in terms of user-perceived behaviour, assuming that the people relying on this are likely to be the same people who have disabled keyword.enabled (which I'm not sure is true, but we have no data...). (3) is the slowest path here. (1) is the most straightforward. (4) might be complex wrt principals and what navigations we allow from where.

At this point, I don't believe any other browser has this behaviour, it causes inconsistencies and confusion like this bug and bug 1679556, and it has extremely limited value in the presence of autofill (ie for anything you've visited before, if you type foo in the address bar, autofill will prefill to foo.com anyway so the alternate fixup won't help). So I'm inclined to suggest (1), but I could be convinced to do any of the other options if I'm missing something.

I think this bug itself should be resolved wontfix. Thoughts?

Fwiw, I think telemetry telling us how often the feature is used would be pointless. We should rather detect whether the feature was "useful", that is a very different task, we should measure whether the user effectively spends time on the page we redirected them to.

I'm more prone to do (4), but I'd probably accept picking (1) with a follow-up bug for the additional hint on the error page, provided it should not take months or years to act on it.
That is, I'd be ok with wontfixing this bug.

(In reply to :Gijs (he/him) from comment #15)

I think this bug itself should be resolved wontfix. Thoughts?

Gijs, Marco, I am fine with closing this one as a WONTFIX.

Given the approaches outlined above, I am for the simplest approach, which would be (1), but that could be discussed in a different bug. Should we file a follow up for discussing the future of alternate fixup?

Yes, but we already have such thing, bug 1679556.

WONTFIX because we are evaluating the removal of alternate fixup (not common to other browser, confusing, has security implications...), and this edge case is a first step towards that direction.