Closed Bug 1294496 Opened 8 years ago Closed 8 years ago

browser.fixup.dns_first_for_single_words prevents location bar search suggestions

Categories

(Firefox :: Address Bar, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: kerncece+redir, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160811030201

Steps to reproduce:

Enable search suggestions in the location bar and set:

    browser.fixup.dns_first_for_single_words = true


Actual results:

Observe how search suggestions in the location bar don't work.


Expected results:

Search suggestions should work even with this option enabled.
Blocks: 1181082
Component: Untriaged → Location Bar
Blocks: 959567
This is not a default option, but it may be used by companies and there's a plan to maybe enable it for ESR. so we should see if it's easily fixable.

Just want to clarify that "don't work" actually means "doesn't fetch suggestions at all", so the UI is not "broken" for the user, just missing a feature.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [fxsearch]
In about:preferences#search both "Provide search suggestions" and "Show search suggestions in location bar results" are checked. So how exactly is this not "broken" and just missing a feature?
(In reply to liquider from comment #2)
> In about:preferences#search both "Provide search suggestions" and "Show
> search suggestions in location bar results" are checked. So how exactly is
> this not "broken" and just missing a feature?

The location bar still lets you navigate and search. "Only" the search suggestions are gone.
I'm putting up a patch, but I'll note that I'm not sure both callsites here need to be updated. That said, with this change, if I type a single word the default entry in the awesomebar popup is still "visit" so I *think* the change in _matchUnknownUrl is correct, but I'm really not familiar with UnifiedComplete.js so this could do with some checking.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Technically the fix looks right, but I'd like us to go through the reason suggestions were disabled here.
The main reason was to avoid disclosing local hosts to remote search engines, so when the word was recognized as a whitelisted host, we didn't do that for privacy reason.
By setting browser.fixup.dns_first_for_single_words, isDomainWhitelisted becomes useless, any word could be a host, and since this pref exists mostly for companies using ESR, we'd end up disclosing every single internal company host to the search engine among the other words users could be looking for.

Enabling search suggestions here sounds like a privacy risk to me, what do you think?

this catch-all pref introduces an unpredictable behavior the urlbar can't cope with. I'd indeed suggest people to not use this pref but to rather use the whitelist prefs. And personally I'd also start a call for its removal.

Regarding the change to _matchKnownUrl, it is wrong either way, cause with your patch the action row it will always read "Search for "word" but sometimes it will visit. without your patch it will always say visit, but sometimes it will search. We could guess that it may be more common to search than to visit, and thus it may be a good change regardless.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Marco Bonardo [::mak] from comment #6)
> Technically the fix looks right, but I'd like us to go through the reason
> suggestions were disabled here.
> The main reason was to avoid disclosing local hosts to remote search
> engines, so when the word was recognized as a whitelisted host, we didn't do
> that for privacy reason.
> By setting browser.fixup.dns_first_for_single_words, isDomainWhitelisted
> becomes useless, any word could be a host, and since this pref exists mostly
> for companies using ESR, we'd end up disclosing every single internal
> company host to the search engine among the other words users could be
> looking for.
> 
> Enabling search suggestions here sounds like a privacy risk to me, what do
> you think?
> 
> this catch-all pref introduces an unpredictable behavior the urlbar can't
> cope with. I'd indeed suggest people to not use this pref but to rather use
> the whitelist prefs. And personally I'd also start a call for its removal.

I would disagree with removing it.

Note that I was initially opposed to introducing it.

However, people convinced me that there are particular situations where there are so many individual hosts that having the individual hosts whitelisted is not easily feasible. There's some history to this discussion in the bug that introduced it.


What I don't understand is that the pref also breaks suggestions for things that clearly *aren't* single word domain lookups (ie current nightly behaviour still breaks suggestions even if you type "foo bar", if the pref is set, IME). Perhaps it would be feasible to only fix that case? I haven't investigated how to do that specifically - I expect the checks for host whitelisting would need to come somewhere else. Can you comment as to whether that would make more sense to you?



> Regarding the change to _matchKnownUrl, it is wrong either way, cause with
> your patch the action row it will always read "Search for "word" but
> sometimes it will visit. without your patch it will always say visit, but
> sometimes it will search. We could guess that it may be more common to
> search than to visit, and thus it may be a good change regardless.

This was not my experience, but it's possible I somehow didn't test it right. Did you test this with the patch, or are you reasoning about how the code "should" work?
Flags: needinfo?(gijskruitbosch+bugs)
ni for comment #7, sorry for the spam.
Flags: needinfo?(mak77)
Attachment #8787786 - Flags: review?(mak77)
(In reply to :Gijs Kruitbosch from comment #7)
> However, people convinced me that there are particular situations where
> there are so many individual hosts that having the individual hosts
> whitelisted is not easily feasible.

I doubt this, since it would be as hard as maintaining a list of the hosts (something anyone would do regardless in a company) and generating the cfg file from it with a few lines script... But let's not go off-topic.

> What I don't understand is that the pref also breaks suggestions for things
> that clearly *aren't* single word domain lookups (ie current nightly
> behaviour still breaks suggestions even if you type "foo bar", if the pref
> is set, IME).

that's due to REGEXP_SINGLEWORD_HOST.test(tokens[0]) that is correct if the next tokens are special chars (filters like %,$,*), but it's incorrect for anything else. We are passing only the first word to isDomainWhitelisted, but if there is more than one word, it can't be a domain.

instead of doing:
let tokens = searchString.split(REGEXP_SPACES); and checking REGEXP_SINGLEWORD_HOST.test(tokens[0])
it should check this._searchTokens.length == 1 && REGEXP_SINGLEWORD_HOST.test(this._searchTokens[0])
That needs a fix and a simple test in some of the tests in unifiedcomplete/ folder.

> > Regarding the change to _matchKnownUrl, it is wrong either way, cause with
> > your patch the action row it will always read "Search for "word" but
> > sometimes it will visit. without your patch it will always say visit, but
> > sometimes it will search. We could guess that it may be more common to
> > search than to visit, and thus it may be a good change regardless.
> 
> This was not my experience, but it's possible I somehow didn't test it
> right. Did you test this with the patch, or are you reasoning about how the
> code "should" work?

I'm reasoning on the code and a string that matches [a-z0-9-]+
I think I know what you mean, due to action uris we enforce an action.

with the patch and the pref set to true, we will always try to search cause we skip the isDomainWhitelisted check, we can't know if it's a host. So we end up in _matchCurrentSearchEngine that forces a search.

without the patch and the pref set to true, isDomainWhitelisted returns always true, so we should always try to visit.

It still looks wrong? off-hand looks like the patch would make impossible to visit the hosts?
Flags: needinfo?(mak77)
considered the complications, atm I'm prone to fix what we found wrong, but wontfix search suggestions. We do that for privacy reasons that are likely more important than having suggestions in some case.
In future we could allow local suggestions and just skip remote ones, if we decide to enable local search history suggestions. So I'd add a comment to bug 1181644 in that regard.
Comment on attachment 8787786 [details]
Bug 1294496 - allow search suggestions for multi-word searches even for whitelisted domains, and test response to "DNS first" pref,

https://reviewboard.mozilla.org/r/76476/#review75468

The patch looks good to me, though it's not fixing this bug, it's fixing something else we found :)
I'd suggest moving the patch to a new bug and wontfix this one, to clarify the scopes and avoid future confusion.
Attachment #8787786 - Flags: review?(mak77) → review+
Blocks: 1301421
Closing per comment #13, pushed the patch from this bug with bug 1301421 as the bug number.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Priority: P2 → --
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: