Closed Bug 1578856 Opened 2 years ago Closed 2 years ago

browser.fixup.dns_first_for_single_words preference no longer works properly

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 71
Iteration:
71.1 - Sept 2 - 15
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: mkaply, Assigned: mak, NeedInfo)

References

(Regressed 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1578856 - browser.fixup.dns_first_for_single_words and the keyword-uri-fixup UI are broken. r=adw
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review
ESR68 patch
24.72 KB, patch
RyanVM
: approval-mozilla-esr68+
Details | Diff | Splinter Review

The preference browser.fixup.dns_first_for_single_words no longer works properly.

When this preference is true, doing single word lookups should never to www.singleword.com.

They should go from a DNS lookup to search.

This is a major issue for enterprise.

There is no urlbar test for dns_first_for_single_words, apart from a search suggestions search, that I'm not even sure it's correct.

The regrange is interesting, both urlbars were using the same code.

So, supposing I'm searching for "test" this is what I see:
with the pref set to true, I get "http://test/" for both fixedURI and preferredURI, "" as keywordAsSent
with the pref set to false, I get fixedURI = "http://test/", preferredURI = "https://www.google.com/search?client=firefox-b-d&q=test" , keywordAsSent = "test"
So the urlbar in the first case produces a visit to "http://test/", in the second case a search because of keywordAsSent.

I suspect the old code was bogus, and let the string pass directly to the docshell, that then was passing it to urifixup, and waiting for the lookup.
As it is now, I don't think it would work, the urlbar code must know what's the final result of the lookup, to generate either a visit or a search result. But I'm also not sure it's a good idea to make it wait for a possibly really slow dns lookup.

The only solutions I see, for now, are:
a) wait for the dns lookup, then return results, if the dns is slow results may not appear (we can timeout)
b) create a new result type that looks like a visit, but in some cases may search (thus it will lie to the user) and would just pass down the string to the docshell

(In reply to Mike Kaply [:mkaply] from comment #0)

The preference browser.fixup.dns_first_for_single_words no longer works properly.

When this preference is true, doing single word lookups should never to www.singleword.com.

Uh? You mean it should never go to a search provider? If you don't want alternate fixup, set browser.fixup.alternate.enabled to false.

I'm very confused about exactly what this bug is about - at the moment it's not clear.

Flags: needinfo?(mozilla)

Put differently: please provide step by step steps to reproduce, with detailed expected/actual results.

It's a regression. So the answer is "It should work like it did before"

In default Firefox, for single words, we go straight to search.

This preference changes it so that we do a DNS lookup for single words first and if that doesn't resolve, then we search.

For some reason, flipping this preference is also making us do the "www.single_word.com" instead of going to search.

All this preference should add is the DNS lookup, it shouldn't change any other behaviors.

Flags: needinfo?(mozilla)

I'll also point out that browser.fixup.alternate.enabled doesn't seem to be working in Firefox right now. If I type in ibm, it searches google for ibm. It does not go to www.ibm.com. (This is on a new profile).

So the fact that flipping this pref suddenly makes us add www/com is odd as well.

(In reply to Mike Kaply [:mkaply] from comment #7)

I'll also point out that browser.fixup.alternate.enabled doesn't seem to be working in Firefox right now. If I type in ibm, it searches google for ibm. It does not go to www.ibm.com. (This is on a new profile).

why should it go to ibm.com instead of searching? I suspect there's some misunderstanding here, afaik alternate is used when visiting "http://word/" doesn't work, then we try the alternate "http://www.word.com/".

why should it go to ibm.com instead of searching? I suspect there's some misunderstanding here, afaik alternate is used when visiting "http://word/" doesn't work, then we try the alternate "http://www.word.com/".

You're correct, that was my confusion.

But either way, this is still broke. The user didn't type in http://foo, they typed in foo.

So alternate should have never even been used.

Also side note, setting browser.fixup.alternate.enabled false doesn't help in this case because then we just end up with DNS failures on all the single words.

let me have a look at this, I have some idea for a workaround, that I want to try. Unfortunately it means we may be lying to the user when this pref is set, we'll show a "Visit" entry in the urlbar, but it may end up searching.

Assignee: nobody → mak77
Status: NEW → ASSIGNED

I think we already implement that? in gKeywordURIFixup... but again, it's possible the current urlbar is skipping that.

(In reply to Mike Kaply [:mkaply] from comment #11)

Might find this interesting from Chrome:

https://www.chromium.org/user-experience/omnibox#TOC-Input-Types-and-Examples

What enterprise settings does Chrome provide to avoid search?

Flags: needinfo?(mozilla)

What enterprise settings does Chrome provide to avoid search?

They don't for this specifcally, but they don't need to as they show the UI letting you know that it's a valid URL and once you visit it, it autocompletes to that single word domain.

Flags: needinfo?(mozilla)

(In reply to Mike Kaply [:mkaply] from comment #14)

What enterprise settings does Chrome provide to avoid search?

They don't for this specifcally, but they don't need to as they show the UI letting you know that it's a valid URL and once you visit it, it autocompletes to that single word domain.

That's exactly what we do... The only reason this preference exists was that people came up who had to use hundreds or thousands of these single-word domains in work settings, and so the notification wasn't deemed a suitable alternative... Has that changed?

Flags: needinfo?(mozilla)

That's exactly what we do... The only reason this preference exists was that people came up who had to use hundreds or thousands of these single-word domains in work settings, and so the notification wasn't deemed a suitable alternative... Has that changed?

No, this is a case where our preference allows us to work better than Chrome.

If we're going to change the behavior on purpose, so be it and we need to relnote it. That's not what happened here.

Flags: needinfo?(mozilla)

Afaict, the notification is also broken currently, from what I can tell it is generated by the docshell, and the urlbar is bypassing the docshell (by passing it a ready-to-use url). That makes me think the test at browser_urlbarSearchSingleWordNotification.js is not particularly good, because it doesn't really simulate the user typing. Setting and confirming the urlbar value is not the same thing.

A nicer long term fix may involve moving all urifixup into a js module and make the urlbar directly use it, the docshell could be simplified, just loading the url it's being passed. Though it's a long project to do.
Short term we should likely restore the old behavior of just sending the bare string to the docshell and let it do its magic.

Duplicate of this bug: 1578961
Iteration: --- → 71.1 - Sept 2 - 15

In the Quantum Bar it's usually the urlbar code that decides whether a search
string should be visited or searched. if dns_first_for_single_words is set,
we can't make a final decision, because that depends on a dns lookup. For now
we don't want to duplicate the docshell code, also because we must keep the
old behavior functioning for cases where the urlbar value is set without input.

Similarly, when the docshell decides to search for a single word host, and a
dns lookup resolves it, it also shows a prompt asking the user if he meant to
visit it instead of searching. Because the urlbar skips the docshell decision
making, we must manually call the fixup prompt code from the urlbar.

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c61214ac69fe
browser.fixup.dns_first_for_single_words and the keyword-uri-fixup UI are broken. r=adw
Regressions: 1581635

https://hg.mozilla.org/mozilla-central/rev/c61214ac69fe

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox71: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Attached patch ESR68 patch — — Splinter Review

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: We broke 2 considerably used enterprise features of the urlbar, that allow to better discern betweek locally resolved hosts and word searches
  • User impact if declined: The browser.fixup.dns_first_for_single_words pref is broken, thus we will always try to visit an invalid host, rather than searching.
    Additionally when searching for a word that can be resolved as a local host, we don't ask to the user if he meant to instead visit that host.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): touching the urlbar behavior always comes with some risks, but we have quite a few urlbar tests. A part of this patch only acts when the specific pref is set, the other parts is pretty much wrapped in a try/catch so it can't break loading pages.
  • String or UUID changes made by this patch: none
Attachment #9093303 - Flags: approval-mozilla-esr68?

STR for QA:
1a. set browser.fixup.dns_first_for_single_words to true
2a. type a word like "test" in the urlbar and press enter
ERa: we should be trying to resolve "test" against the DNS server, when that fails we'll fallback to a search

1b. set browser.fixup.domainwhitelist.localhost to false
2b. type localhost in the urlbar and press enter
ERb: we should run a search for localhost, but also show a "did you mean to visit localhost instead" notification bar. When confirming the button in the notification, the browser.fixup.domainwhitelist.localhost pref should be flipped to true

Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Can we nominate this for Beta uplift also?

Flags: needinfo?(mak77)

yes, but it doesn't seem strictly necessary, these features are mostly for enterprise, that should be on ESR, that's why I didn't take a risk of further uplifts.

Flags: needinfo?(mak77)

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

STR for QA:
1a. set browser.fixup.dns_first_for_single_words to true
2a. type a word like "test" in the urlbar and press enter
ERa: we should be trying to resolve "test" against the DNS server, when that fails we'll fallback to a search

1b. set browser.fixup.domainwhitelist.localhost to false
2b. type localhost in the urlbar and press enter
ERb: we should run a search for localhost, but also show a "did you mean to visit localhost instead" notification bar. When confirming the button in the notification, the browser.fixup.domainwhitelist.localhost pref should be flipped to true

Hello!
Using the above STR the issue is verified fixed with Firefox 71.0a1 (20190904163258) on Windows 10x64, macOS 10.14 and Ubuntu 18.04. Leaving the qe+ flag if this receives an uplift to esr.

status-firefox71: fixed → verified

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

yes, but it doesn't seem strictly necessary, these features are mostly for enterprise, that should be on ESR, that's why I didn't take a risk of further uplifts.

I asked Mike and he agrees that we should aim for parity unless there's a strong reason not to. Please request Beta approval on this too.

Flags: needinfo?(mak77)

Why is parity between Firefox 70 and ESR so important, compared to having it in Firefox 71? Is there a specific reason these features are critical for Firefox 70?

Flags: needinfo?(mak77)
Flags: needinfo?(mozilla)

We want parity between Firefox releases and their corresponding ESRs.

69.1 ESR -> 69
69.2 ESR -> 70
69.3 ESR -> 71

So that we can talk about them being fixed in both locations.

Typically we fix things on beta and uplift to ESR. This was a little backwards :)/

And we have tons of folks that use RR, not just ESR in enterprise.

Flags: needinfo?(mozilla)

(In reply to Mike Kaply [:mkaply] from comment #30)

Typically we fix things on beta and uplift to ESR. This was a little backwards :)/

It's the first time I hear about this "rule"; I mean, surely for a critical fix we want it on beta and ESR, for an enterprise fix I still don't see why that would exist. Beta has its own risk management that is not the same as ESR, because population is very different, a rule that forces them to pick the same fixes would be (imo) wrong.

And we have tons of folks that use RR, not just ESR in enterprise.

Not much different than a normal user from a risk management point of view. With shorter cycles it'd be a 4 weeks difference.

Anyway, since you care so much, I'll check if this applies cleanly to beta.

Comment on attachment 9092489 [details]
Bug 1578856 - browser.fixup.dns_first_for_single_words and the keyword-uri-fixup UI are broken. r=adw

Beta/Release Uplift Approval Request

  • User impact if declined: We broke 2 considerably used enterprise features of the urlbar, that allow to better discern betweek locally resolved hosts and word searches.
  • 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: Already done
  • List of other uplifts needed: Bug 1581635
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): One of the 2 disabled features prompts the user whether he wants to visit "host" after running a search for it, when the local dns can resolve "host". In general it should not be a problem, but since this is skipping a part of the Nightly testing cycle, there may be unknown interactions with third party dns resolvers (security software?). That said, the feature was enabled until Firefox 68, so the risk may be contained.
  • String changes made/needed:
Attachment #9092489 - Flags: approval-mozilla-beta?

Maybe a bit risky but I'm not sure that waiting for 71 will get us any better feedback from users. So I'm leaning towards taking this but I think we could go either way. Ryan, what do you think about taking this in ESR ? Go for it now, or wait till the next ESR? We could also try this in beta 10, and see how it goes before accepting it into ESR.

Flags: needinfo?(ryanvm)

I agree with you that we're not likely to get much more useful feedback from this on Nightly. I'd also feel better about taking it on ESR with it getting some testing on Beta (since there's basically little to no ESR prerelease testing). If you're OK taking this on Beta for 70 still, that'd WFM :)

Flags: needinfo?(ryanvm)

Comment on attachment 9092489 [details]
Bug 1578856 - browser.fixup.dns_first_for_single_words and the keyword-uri-fixup UI are broken. r=adw

OK for uplift for beta 11. That may give us more confidence for ESR uplift. Let's see how this goes.

Attachment #9092489 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hello!
The issue is verified fixed using STR from comment 24 having expected results with Firefox 70.0b11 (20190928000111) from comment 36 on Windows 10x64, macOS 10.14 and Ubuntu 18.04.

status-firefox70: fixed → verified
Comment on attachment 9093303 [details] [diff] [review]
ESR68 patch

Fixes a URLbar regression which can cause issues for enterprise users. Verified on Nightly and Beta. Approved for 68.2esr.
Attachment #9093303 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

The issue is verified with Firefox 68.2.0esr (20191001163956) on Windows 10x64, Ubuntu 18.04 and macOS 10.14.

Status: RESOLVED → VERIFIED
status-firefox-esr68: fixed → verified
Flags: qe-verify+

I tried to make a new issue but some CC email "didn't match anything" and pressing Back emptied all the fields, just wanted to say that, mozregression pointed to this bug and commit c61214ac69fe37e535a28fc448dc7be20347d585 as to why browser.fixup.dns_first_for_single_words=false acts as if it's true thus DNS-looking up single words.

Flags: needinfo?(mak)

(In reply to jhdskag3 from comment #41)

I tried to make a new issue but some CC email "didn't match anything" and pressing Back emptied all the fields, just wanted to say that, mozregression pointed to this bug and commit c61214ac69fe37e535a28fc448dc7be20347d585 as to why browser.fixup.dns_first_for_single_words=false acts as if it's true thus DNS-looking up single words.

We really need some more details here, so please do file the bug. Do you still get a search page as expected? The DNS lookup is still somewhat expected (to show you the notification bar saying "hey, did you mean to go to <word> ?" if the server exists; it's just not supposed to block the loading of the search page.

Flags: needinfo?(jhdskag3)

(In reply to u652029 from comment #41)

I tried to make a new issue but some CC email "didn't match anything" and pressing Back emptied all the fields, just wanted to say that, mozregression pointed to this bug and commit c61214ac69fe37e535a28fc448dc7be20347d585 as to why browser.fixup.dns_first_for_single_words=false acts as if it's true thus DNS-looking up single words.

That's expected.
When browser.fixup.dns_first_for_single_words is true, we first dns-lookup and then if that fails we execute a search
When browser.fixup.dns_first_for_single_words is false, we first execute a search, and then dns-lookup, if the lookup is successful we ask to the user whether he wanted to go to "typedstring".

This behavior always existed, but we broke it when we introduced the new urlbar code, thus we just fixed a regression.
As far as I know, the latter feature never had a disabling pref.

Flags: needinfo?(mak)
You need to log in before you can comment on or make changes to this bug.