User Agent: Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111011 Firefox/9.0a2 Build ID: 20111011042016 Steps to reproduce: 1. Enter a number in the location bar (e.g. 1234) 2. Press Enter or the Go button Actual results: Nothing happens, except that the location bar is blurred. If there already was a page in the tab, it doesn't disappear. Notable exception: 0 seems to try to access http://0/ which for some reason is the same as http://localhost/. This behaviour is also unexpected. Expected results: A search with the "location bar search engine" (Google, by default) should have been loaded for the number in question.

Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111020 Firefox/10.0a1 Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111021 Firefox/10.0a1 Indeed, typing numbers and enter does not yield any result. This is an old issue, but couldn't find any other relevant bug related to this issue. I can reproduce it on Firefox 4 as well. Setting to new and waiting for extra feedback if invalid for some reason.

hi firefox doesn't load a page as it says "Unable to connect - Firefox can't establish a connection to the server at 9211." this is because when a number is entered in the location bar, it is treated as an address. this may be the intended behavior. can you confirm whether it is indeed a bug and if it is, what should be the behavior?

@abhishek > firefox doesn't load a page as it says "Unable to connect - Firefox can't establish a connection to the server at 9211." That is not what happens to me. I just get a blank page, same as if entering "about:blank" in the location bar, except that the number stays there.

i feel that some result should be displayed even for numbers. other browsers like chrome display search results. so i think, first it should be checked whether the number entered forms a valid address. if it doesn't, then it should be searched with the location bar search engine. i think i'll start working on it.

How does this patch interact with a user typing in an IP address into the location bar? I.e. 192.168.0.1 or whatever to get to a local router, or 74.125.224.71 to get to google (probably depending on where it the world you are etc)...

Oh, duh, this patch checks for dots...

Comment on attachment 657220 [details] [diff] [review] patch causes number keywords in locationbar to be searched as normal terms [backed out] r=jst

Comment on attachment 657220 [details] [diff] [review] patch causes number keywords in locationbar to be searched as normal terms [backed out] Please replace the tab stops with spaces.

The fix in this bug is ready to land now, no?

With comment 9 addressed, yeah. A test wouldn't hurt either, but I'm not sure there are any existing docshell-level tests for any of this fixup code.

I think this brings a usability regression when e.g. you have the URL 'localhost/blah' loaded and you then delete 'blah' - instead of going to URL localhost, urlbar searches for keyword 'localhost'.

Backed out due to bug 809745: https://hg.mozilla.org/integration/mozilla-inbound/rev/54ffff3e4e83

Is this still being worked on?

This is in addition to part 1. This patch checks to see if the host has been visited, and if so it will not do the URI fixup. The next step here will be to kick off an async DNS lookup that will see if the originally requested URI has a response, and if so to show an infobar that will allow the user to visit the URI. After the visit, all subsequent attempts will go directly to the URI since the hostname will be present in the moz_hosts table.

Comment on attachment 8412388 [details] [diff] [review] Check if host has been visited before switching to a search, part 2 Review of attachment 8412388 [details] [diff] [review]: ----------------------------------------------------------------- jst, what do you think about this approach?

Comment on attachment 8412388 [details] [diff] [review] Check if host has been visited before switching to a search, part 2 Review of attachment 8412388 [details] [diff] [review]: ----------------------------------------------------------------- jst, what do you think about this approach?

Marco, what do you think about this?

Comment on attachment 8412388 [details] [diff] [review] Check if host has been visited before switching to a search, part 2 Review of attachment 8412388 [details] [diff] [review]: ----------------------------------------------------------------- This is an interesting idea. I think it's right: the criterion that should ultimately determine whether a string is a search isn't the characters it contains but whether it resolves to a hostname or address. (Would these patches also handle "myIntranet/foo"? It doesn't look like it. It'd be nice to try to parse the hostname from a longer string rather than looking up the whole string.) But, hasHostBeenVisited does sync I/O, which isn't good. (I presume KeywordURIFixup is called on the main thread?) And I think a better UX would be to just visit the page if the lookup was successful instead of asking the user to go there. (Maybe you could ask for forgiveness after the visit and then add the hostname to a blacklist, but I'm not sure even that's necessary.) I realize fixing both of those things is probably much harder, though, since you'd have to introduce asynchronicity into this part of the load process. And my UX suggestion has its own problems since now you're waiting on a name lookup potentially every time you hit enter in the location bar instead of immediately loading Google.

My approach mirrors the implementation behavior of Chrome, which I find is the best-in-market right now. Redirecting the page request without user interaction, potentially after the search page has finished, is confusing to the user and will only make the whole process feel longer. Will the user begin to wait after the search page has finished loading just to make sure that it doesn't navigate away on its own in the next 500+ ms? I don't think we can introduce *any* asynchronicity into this feature. What we can do though is try to remove disk I/O if possible, by using an in-memory DB lookup. I have been told that the inline-autocomplete uses an in-memory DB. These patches should handle myIntranet/foo, as they check the hosts table to see if the host has been visited. This is similar in results as viewing the Page Info dialog and seeing how many times this host has been visited.

Comment on attachment 8412387 [details] [diff] [review] Original patch, part 1 Review of attachment 8412387 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDefaultURIFixup.cpp @@ +840,5 @@ > (spaceLoc < colonLoc || quoteLoc < colonLoc) && > (spaceLoc < qMarkLoc || quoteLoc < qMarkLoc)) || > + (dotLoc == uint32_t(kNotFound) && > + colonLoc == uint32_t(kNotFound) && > + qMarkLoc == uint32_t(kNotFound)) || kNotFound is -1, what's the point of casting it to unsigned?

Comment on attachment 8412388 [details] [diff] [review] Check if host has been visited before switching to a search, part 2 Review of attachment 8412388 [details] [diff] [review]: ----------------------------------------------------------------- I think the approach is good: - check if the token is whitelisted, if so just load that host - if not whitelisted do the search by default and in the meanwhile check history and dns asynchronously - if either of the 2 succeeds, ask the user if he meant to visit the host - if positive answer, add to the whitelist Did I read it correctly? This is likely going to work fine cause the whitelist should commonly be very small, probably a handful. The only problematic thing is reading the whitelist on startup, we don't have very good generic solutions for that, we may just start an async load of it, and if unfortunately the user asks before it's loaded, will just fallback to the standard path of asking him. Should be a rare case though, and it's measurable through telemetry. I don't think a blacklist would be good cause I suppose it would grow a lot (many users just type words in the locationbar to search), searching through it may be expensive and indexing would grow it even more. The implementation doesn't seem perfect for this approach though, I'm not even sure the APIs we have (defalturifixup) are good enough for it, we would ideally want an async reply to our request, to avoid jank, also for the dns resolution (that should ideally happen on a separate thread). We want an immediate answer (is whitelisted?) and an async answer (if not whitelisted, is it a valid host?). ::: docshell/base/nsDefaultURIFixup.cpp @@ +832,5 @@ > + "SELECT host || '/' FROM moz_hosts " > + "WHERE host=?1 " > + "LIMIT 1" > + ), getter_AddRefs(stmt)); > + NS_SUCCEEDED(rv); docshell cannot depend on Places like this, you may #ifdef MOZ_PLACES some code using History.cpp, and do an asynchronous check through a dedicated API (that should be added). Definitely this can't be synchronous, nor it should run a raw query like this. Mobile may find this useful as well, and they have their own globalhistory implementation. And no, autofill doesn't use a temp table, it executes an async query. the initial idea was to make it use a temp table, but it was discarded for memory usage reasons (Chrome uses a small temp table for a part of inline completion IIRC).

Comment on attachment 8412980 [details] [diff] [review] Run an async lookup to see if the host can be resolved, and offer the user to visit that host if so, part 3 Review of attachment 8412980 [details] [diff] [review]: ----------------------------------------------------------------- I think for a first experimental implementation I'd just query Places moz_host from this browser code, before putting global history stuff deep in the urifixup path (I'm not even sure we want a so generic util to have its results changed based off history, that is a volatile and very personal kind of data). I don't recall if the infobar we are opening here is tab modal, since the process is async, there's good probability the user may have: 1. moved to another tab/window 2. loaded another url in this same tab Doesn't look like there's protection for 2 at least, here. Some comments (though I didn't review this line by line, I just skimmed over quickly) ::: browser/base/content/browser.js @@ +768,5 @@ > + let dnsService = Cc["@mozilla.org/network/dns-service;1"].getService(Ci["nsIDNSService"]); > + if (!browser || !dnsService) > + return; > + > + let listener = { I think you should just add the [function] decorator to nsIDNSListener, if netwerk team is fine with that. @@ +814,5 @@ > + } > + }; > + > + let threadManager = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager); > + dnsService.asyncResolve(data, 0, listener, threadManager.mainThread); Services.tm.mainThread

You'd skip the search page entirely, not load it and then redirect to the looked-up host.

(In reply to Drew Willcoxon :adw from comment #32) > You'd skip the search page entirely, not load it and then redirect to the > looked-up host. Looking up the host is what we do now, it is slow and makes the UX terrible :)

I'll see what I can do about having an in-memory whitelist that is loaded after the browser has started up, but I'm not sure how to separate the whitelist from nsdocshell, as that is where we need to know if we should redirect to the search or leave it alone.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33) > Looking up the host is what we do now, it is slow and makes the UX terrible I don't get it... if looking up the host is what we do now, then what's the point of your patches, which look up the host?

(In reply to Drew Willcoxon :adw from comment #35) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33) > > Looking up the host is what we do now, it is slow and makes the UX terrible > > I don't get it... if looking up the host is what we do now, then what's the > point of your patches, which look up the host? Sorry, I should have been more clear. We currently look up the host prior to showing the search page, which is terribly slow for the non-hostname case. My patch delays looking up the hostname until after the search page has started loading (and then iff the hostname lookup succeeded do we show the notification bar).

Added to Iteration 33.3

(to be clear for posterity, there was an email discussion about prioritizing this, and that's why I'm "stealing" this from Jared) (In reply to Marco Bonardo [:mak] from comment #31) > Comment on attachment 8412980 [details] [diff] [review] > Run an async lookup to see if the host can be resolved, and offer the user > to visit that host if so, part 3 > > Review of attachment 8412980 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think for a first experimental implementation I'd just query Places > moz_host from this browser code, before putting global history stuff deep in > the urifixup path (I'm not even sure we want a so generic util to have its > results changed based off history, that is a volatile and very personal kind > of data). Marco, just so I'm sure I understand - this part of your comment that I quoted is actually still about attachment 8412388 [details] [diff] [review], right? I don't see any history bits in this attachment. I'm hesitant about the history bits as a whole anyway, because: 1) we want to make a decision as to whether we should first query the search provider (while doing the DNS lookup) synchronously 2) we can't make the history call synchronously (if I read your comments correctly) 3) the history bits will be tricky from a codebase modularity perspective 4) the history bits make this less predictable (see your last comments in () ) - if you've previously navigated to http://localserver/ (especially if from the infobar prompt where you told the browser that was what you wanted), and you then clear history and suddenly get redirected to the search provider when typing in 'localserver', that is probably unexpected behaviour. So I'm leaning towards doing a whitelist and leaving out the places/history bits entirely. This will also simplify getting this to the finish line. Does that seem OK to you? For the whitelist... is using the pref system a terrible idea? That gets round (or ignores, to a certain degree...) the perf issue, at least. We could even, because we know that for these items, (1) there are no spaces, and (2) there is just text + numbers, perhaps use a tree (ie "browser.urlbar.hostwhitelist.mylocalmachine", "browser.urlbar.hostwhitelist.anotherlocalmachine") instead of a single list pref? If we were worried about there being 100 entries, I would be hesitant, but I think anything bigger than 10 items would be extremely rare. Or do you think there is / should be a different place for this data? Finally (and this is not really a question, although I'd appreciate solutions), I know that by default my ISP (and it is not unique in this respect) intercepts non-resolving DNS requests and redirects them to its own branded search page. I wonder if we can figure out a way to detect that, because getting this infobar in those cases ("false positive") would be annoying for the user... (I'm guessing Chrome solves this by always using its own DNS servers, but I'm not sure)

(In reply to Marco Bonardo [:mak] from comment #29) > Comment on attachment 8412387 [details] [diff] [review] > Original patch, part 1 > > Review of attachment 8412387 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: docshell/base/nsDefaultURIFixup.cpp > @@ +840,5 @@ > > (spaceLoc < colonLoc || quoteLoc < colonLoc) && > > (spaceLoc < qMarkLoc || quoteLoc < qMarkLoc)) || > > + (dotLoc == uint32_t(kNotFound) && > > + colonLoc == uint32_t(kNotFound) && > > + qMarkLoc == uint32_t(kNotFound)) || > > kNotFound is -1, what's the point of casting it to unsigned? All of these are uint32_ts. I don't understand why. Boris, you swapped these from (PR)int32s, can you explain why this was necessary? I'm... a little confused. :-)

(bug 550458, fwiw)

Marco, Boris, does this look right to you for the first portion (ie without the notification bar UI etc.) ? In particular, very much not sure if anything acceptable as an (IDN or not) host will pass if sent to the prefs service, and I copied some of the idn service handling thingies from nsStandardURL, but not sure I got it (and the destructor bit) right.

I don't actually know if I need to rev the uuid for this or not...

Comment on attachment 8454392 [details] [diff] [review] add function decorator to nsIDNSListener, Review of attachment 8454392 [details] [diff] [review]: ----------------------------------------------------------------- I don't know if you need to bump the uuid or not either for this. I bet benjamin knows :)

Comment on attachment 8454388 [details] [diff] [review] part 1: entering numbers+Enter in the location bar should bring search results immediately if domain is not whitelisted, f?bz (In reply to :Gijs Kruitbosch from comment #41) > Created attachment 8454388 [details] [diff] [review] > part 1: entering numbers+Enter in the location bar should bring search > results immediately if domain is not whitelisted, f?bz > > Marco, Boris, does this look right to you for the first portion (ie without > the notification bar UI etc.) ? In particular, very much not sure if > anything acceptable as an (IDN or not) host will pass if sent to the prefs > service, and I copied some of the idn service handling thingies from > nsStandardURL, but not sure I got it (and the destructor bit) right. Hrmpf, well, at the very least, I was assuming this only got hostname-only bits, but it doesn't - things like foo/bar also hit this path. I guess I should just use NS_NewURI instead. Going to nix the feedback flags because this moots my second question - if people have answers to the first question, that'd still be very interesting :-)

Checkpointing this part.

(In reply to :Gijs Kruitbosch from comment #38) > Marco, just so I'm sure I understand - this part of your comment that I > quoted is actually still about attachment 8412388 [details] [diff] [review], > right? I don't see any history bits in this attachment. At the time the patch was querying places.sqlite to figure if an host was known, my comment was referred to that. If it's not doing anymore, you can ignore it. > 2) we can't make the history call synchronously (if I read your comments > correctly) right. > 3) the history bits will be tricky from a codebase modularity perspective you would need a generic API that each platform (mobile, desktop,...) has to implement with its own history service. > So I'm leaning towards doing a whitelist and leaving out the places/history > bits entirely. This will also simplify getting this to the finish line. Does > that seem OK to you? np on my side. If it should ever be needed the above thoughts would be valid. Regardless, I'm not sure you can go synchronous here, in any case, you will always have to read your whitelist from the disk, and that should be async, or you should do a guess until the data is ready. > For the whitelist... is using the pref system a terrible idea? Prefs are a perf hog, though we already pay the price, it depends on the size of the data. If, as you say, that data is limited to < 10 entries, it might be fine.

Comment on attachment 8454392 [details] [diff] [review] add function decorator to nsIDNSListener, revving the UUID on trunk has no harmful side effects, so do it freely. It's only once we get to beta that such questions get interesting.

> can you explain why this was necessary? It wasn't _necessary_, but it made the code simpler. Does the comment starting "Note:" right above this code not explain it? Otherwise, the code would have looked like this: if (((spaceLoc != kNotFound && spaceLoc < dotLoc) || ((quoteLoc != kNotFound && quoteLoc < dotLoc))) && etc

Er, actually, that might be wrong too. The point is, it would be complicated enough that it would be hard to tell what's going on. ;)

Comment on attachment 8454392 [details] [diff] [review] add function decorator to nsIDNSListener, remote: https://hg.mozilla.org/integration/fx-team/rev/83a1a4866c9b

Ugh. So I was planning on having another patch up tonight, but I'm running into roadblocks. Roughly, the observer that pops up the notification bar needs 3 bits: 1) something resembling the original URI (e.g. "localhost"), to load if the user says "yes, I want the original URI" 1.1) the host of (1) (implicitly: the parsed URI version that the uri fixup service would return if not doing the keyword search, so that we can whitelist the host of said URI, and display it in the message) 2) the current and/or fixed up URI. At the point where the notification fires, the gBrowser.currentURI is still the currently-loaded page (in a new tab, for instance, about:newtab). There doesn't seem to be a way to figure out that the fixed up form is https://www.mysearchengine.com/search?q=localhost (this to combat issue (2) identified in comment #31) 3) the current docshell so we know what browser to use (currently passed as the subject of the observer notification) This is a problem because the observer notification only lets me pass 2 of these. I can deduce (1) and (2) from the original input by calling createFixupURI myself (twice, with different flags), but that means we're calling the same method 2 extra times to see what happened the first time we called it. The final kicker is that actually, the observer fires whenever the initial user input isn't a fully valid URI, so even in cases like "www.mozilla.org" or "ttp://www.mozilla.org/", where we don't do a search (but with these patches, still show a notification bar telling the user "oh, are you sure you don't want to go to www.mozilla.org?"). I'm not sure what the best way to fix all this is. The root cause is that consumers care about the internals of createfixupuri. While I can probably hack around with some effort, the cleanest way would be to allow createfixupuri to provide more info about what it did to fix the URI. That could either be a new interface or we could add optional out params that docshell then transforms into a new interface to pass as the subject of the observer notification, including a reference to the docshell, the uri without keywords and the uri with keywords (where either might be null). Both of those sound heavy handed. The other thing that almost seems like it might be worth it is to just do the whole thing in JS and bypass the urifixup service completely. :-\ Boris, thoughts on these options? I don't find any of them particularly attractive, so I'm kind of hoping I'm just missing something.

We could invent an interface to pass to the observer notification that has multiple things in it, if that helps. Past that, I have no strong opinions. The exact breakdown of work between "ui code" and "URI fixup C++" has never made _that_ much sense to me: the latter is supposed to be code that all Gecko-based browsers want to have in common or something, but as a result (and because we want to allow users to control it, admittedly) it ends up with lots of frobs to control its behavior... I'd be just fine with changing the createFixupURI API to return more information about what was passed in, what the output is, and how the output came to be that way, if that would solve the issues you're running into.

Right. So this is what I concocted in the end. It's basically using a structure to pass most of the results of the decisions made in the fixup component along. I added bools for whether a protocol was detected, a fully-qualified domain name was there, and whether we ended up using the keyword service, for convenience more than anything. The browser implementation uses the latter, but for practical purposes, I *think* it is equal to whether or not fixedURI matches preferredURI... In any case, certainly the former two (protocol/FQDN status) could in principle be omitted. Boris, you already helped fix one use-after-free bug in this patch... but please could you review the rest extra-carefully as well? I still don't fully trust myself in these waters....

And this would be the browser part. Tested with a simple IDN case (specifically, the one from bug 381344, using /etc/hosts), and it seems to work well. Marco, can you review this? :-)

Hrmpf, and while writing a test I just realized I regressed the observation of the FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP flag in bug 982428. :-(

This fixes the regression I noted in the previous comment (by adding a check for the flag at the bottom of the fixup code again - it was previously kept track of in the local variable, but now we're using a static which only tracks the pref). I've added a fairly comprehensive test (it only doesn't care about protocol typo stuff because there's a separate test for that).

And a browser frontend test as well. Try push with all 3 patches: https://tbpl.mozilla.org/?tree=Try&rev=4716998a16a8

(In reply to :Gijs Kruitbosch from comment #58) > Created attachment 8456289 [details] [diff] [review] > add browser test for the notification for localhosts, > > And a browser frontend test as well. Try push with all 3 patches: > https://tbpl.mozilla.org/?tree=Try&rev=4716998a16a8 Hey look, I broke large parts of the world on Linux... but not on OS X. :-\ Investigating...

I believe I've found and fixed the issue, and added tests for it. In case interdiff is broken, here's what I did: http://pastebin.mozilla.org/5559600 . Basing this on the stack at https://tbpl.mozilla.org/php/getParsedLog.php?id=43843785&tree=Try&full=1#error1 . I do wonder why we're hitting this case (empty string passed as first argument to nsIURIFixup) on Linux but not on OS X...

Renewed try push: remote: https://tbpl.mozilla.org/?tree=Try&rev=dbb416e2b4c1 Next issue: this is leaking on OS X debug on some runs. Not sure why yet...

Comment on attachment 8456374 [details] [diff] [review] part 1: entering numbers+Enter in the location bar should bring search results immediately if domain is not whitelisted, >+++ b/docshell/base/nsDefaultURIFixup.cpp >+nsDefaultURIFixup::GetFixupURIInfo(const nsACString& aStringURI, uint32_t aFixupFlags, >+ FileURIFixup(uriString, getter_AddRefs(uri)); >+ if (uri) >+ { >+ info->mPreferredURI = uri; >+ NS_IF_RELEASE(uri); How about replacing those two lines with: uri.swap(info->mPreferredURI); ? The code you had double-released "uri"; I can only assume our tests don't exercise this codepath. :( >+ rv = NS_NewURI(getter_AddRefs(uri), uriString, nullptr); >+ if (NS_SUCCEEDED(rv) && uri) { If NS_SUCCEEDED(rv), then uri better not be null... >+ info->mInputHasFQDN = host.FindChar('.') != kNotFound; It's a bit odd to do this, since "bugzilla.mozilla" is not an FQDN... and in general an FQDN would _end_ with a dot (not that anyone does that). How about we just call it inputHasDot if that's what it means? >+ uri = nullptr; >+ nsresult rv = KeywordToURI(uriString, aPostData, getter_AddRefs(uri)); getter_AddRefs nulls out its argument, so no need for the explicit set to nullptr. >+ nsIURI* uriWithProtocol; >+ rv = CreateURIWithProtocol(uriString, &uriWithProtocol); This leaks uriWithProtocol. You want that to be an nsCOMPtr<nsIURI> and use getter_AddRefs. Also, you ignore the rv here, so just make CreateURIWithProtocol() a void method? And it could use a better name, but I'm not sure what. Maybe FixupURIProtocol? > void nsDefaultURIFixup::KeywordURIFixup(const nsACString & aURIString, >+ nsAutoCString pref(asciiHost); >+ pref.InsertLiteral("browser.fixup.domainwhitelist.", 0); Reversing the order of those two would be nice. So first init to the literal, then append the host. >+nsDefaultURIFixupInfo::nsDefaultURIFixupInfo(const nsACString& stringURI): aStringURI. Or better yet aOriginalInput as in the header. >+nsDefaultURIFixupInfo::GetConsumer(nsISupports** aConsumer) >+ NS_ENSURE_ARG_POINTER(aConsumer); There's really no need to null-check outparams, here or elsewhere. >+++ b/docshell/base/nsDocShell.cpp >+ fixupInfo->SetConsumer(GetAsSupports(this)); >+ >+ if (NS_SUCCEEDED(rv) && fixupInfo) { That seems backwards: you already used the value, then you check it? Please fix. r=me with the above fixed.

Thanks for the thorough review! This was very helpful. ¬ I've made all the fixes you've suggested, I believe, with these adjustments (#2 and #4 probably being the most controversial): 1) I've added a test for the file-path-to-uri case, which indeed crashed on my old patch, and works on my new patch. 2) I've renamed the FQDN flag to inputHostHasDot. This is still false for (e.g.) the file path (even where it includes a dot!). I think this is more useful than a straight up equivalence with whether or not the entire input string had a dot. 3) I've renamed CreateURIWithProtocol and adjusted its calling so it doesn't leak. I verified locally that this fixes the mochitest-a11y leak (I'll do a try push in a bit). 4) I've *not* made CreateURIWithProtocol a void method. Because in fact, its rv is the last assignment to rv, and is returned if keywords are turned off, which is what makes really invalid stuff, err, really invalid (like NS_ERROR_MALFORMED_URI invalid). I hope I've understood that correctly, can you verify and/or tell me I'm wrong?¬ 5) I've left the out pointer checks in CreateExposableURI alone, as I'm not touching that part of the code, and removed all the other ones, I think. Does that make sense or do you want me to remove them, too? 6) I added a test for the host/foo.txt case, which shouldn't invoke a search URL (and doesn't, but it was a little tricky to adapt the test to check that case).

Updated try push: https://tbpl.mozilla.org/?tree=Try&rev=d79890c38984

Comment on attachment 8456827 [details] [diff] [review] part 1: entering numbers+Enter in the location bar should bring search results immediately if domain is not whitelisted, >+ nsresult rv = KeywordToURI(uriString, aPostData, getter_AddRefs(uri)); >+ if (NS_SUCCEEDED(rv)) { I don't trust KeywordToURI (which calls into some sort of weird nsISearchSubmission code) to not return NS_OK and a null URI. So here we do want to null-check the URI. > Because in fact, its rv is the last assignment to rv, and is returned if > keywords are turned off OK, that's a pretty non-local effect. Can you at least add a comment to that effect? The rest looks great, thanks!

With the last two nits addressed.

Comment on attachment 8457617 [details] [diff] [review] part 1: entering numbers+Enter in the location bar should bring search results immediately if domain is not whitelisted, Carrying over r+ on this portion. I'm going to be on PTO until Monday evening, with sporadic if any internet access. I've been told we basically really want this to make 33, and the strings in the browser portion mean an uplift isn't viable. Assuming the frontend bits here get r+, Marco, can you push this?

...and if the FE parts still need further work, are the strings stable enough that we can just go ahead and pre-land them? That will take the "landing strings in Aurora" problem off the table.

Mak: also, if there's more work needed after a review pass, let me know and I'll find someone to help finish it.

Comment on attachment 8455310 [details] [diff] [review] part 2: use the notification from the browser UI in order to let the user navigate to the original URI, Review of attachment 8455310 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +747,5 @@ > > +const gKeywordURIFixup = { > + observe: function(fixupInfo, topic, data) > + { > + if (topic != "keyword-uri-fixup" || !this._dnsService) Both of these checks are pointless 1. it's not up to this code to test observer service is doing its job. 2. if dnsService can't be instantiated the browser has more serious issues than printing a this._dnsService is null in the console (it might instead be useful to not hide that error at all) @@ +793,5 @@ > + let hostName = alternativeURI.host; > + // and the ascii-only host for the pref: > + let asciiHost = alternativeURI.asciiHost; > + > + let onLookupComplete = (inRequest, inRecord, inStatus) => { nit: according to recent discussions, we don't decorate arguments anymore in "modern" js (regardless "in" prefix would not even follow the previous "a" prefix coding style) @@ +811,5 @@ > + if (!notificationBox) > + return; > + let n = notificationBox.getNotificationWithValue("keyword-uri-fixup"); > + if (n) > + return; is the temp var really needed? why not just if (notificationBox.getNotificationWithValue("keyword-uri-fixup")) return; @@ +828,5 @@ > + try { > + Services.prefs.setBoolPref(pref, true); > + } catch (ex) { > + Cu.reportError(ex); > + } too much defensive coding, we don't usually try/catch simple calls like setBoolPref, and even if it would fail nothing serious happens, nothing more serious than the global broken status the browser would be, at least. @@ +837,5 @@ > + label: gNavigatorBundle.getString("keywordURIFixup.dismiss"), > + accessKey: gNavigatorBundle.getString("keywordURIFixup.dismiss.accesskey"), > + callback: function() { > + let notification = notificationBox.getNotificationWithValue("keyword-uri-fixup"); > + if (notification) may this really be null since this is the callback from a button of that exact notification? sounds like a pointless check @@ +843,5 @@ > + } > + } > + ]; > + n = notificationBox.appendNotification(message, "keyword-uri-fixup", null, > + notificationBox.PRIORITY_INFO_HIGH, buttons); nit: I don't like single letter var names for anything else than indices @@ +847,5 @@ > + notificationBox.PRIORITY_INFO_HIGH, buttons); > + n.persistence = 1; > + }; > + > + this._dnsService.asyncResolve(hostName, 0, onLookupComplete, Services.tm.mainThread); I think you can omit mainThread, due to http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#695 js callers should always get notifications on the main thread regardless. @@ +853,5 @@ > +}; > + > +XPCOMUtils.defineLazyServiceGetter(gKeywordURIFixup, "_dnsService", > + "@mozilla.org/network/dns-service;1", > + "nsIDNSService"); nit: looks like this might just be gDNSService and gKeywordURIFixup could be a plain function (since nsIObserver has the [function] decorator. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +113,5 @@ > crashedpluginsMessage.learnMore=Learn More… > > +# Keyword fixup messages > +keywordURIFixup.message=Did you mean to go to %1$S? > +keywordURIFixup.goTo=Yes, take me to %1$S Why did you use numbered entries here, %S should be enough since there's just one replacement. This also wants a LOCALIZATION NOTE explaining what %S will be replaced with.

Comment on attachment 8455310 [details] [diff] [review] part 2: use the notification from the browser UI in order to let the user navigate to the original URI, >+ let notificationBox = gBrowser.getNotificationBox(browser); >+ if (!notificationBox) >+ return; What's the point of this check? >+ loadURI(alternativeURI.spec, null, null, false); Can you use openUILinkIn instead of loadURI? It's much less obscure on the call side with regards to the arguments it takes. You could also just drop "null, null, false", because these are the default values.

Comment on attachment 8456289 [details] [diff] [review] add browser test for the notification for localhosts, Review of attachment 8456289 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js @@ +4,5 @@ > +"use strict"; > + > +let valueToOpen; > +let expectSearch = false; > +let expectNotification = false; there doesn't seem to exist a relation between tests such that these should be shared as globals. Indeed looks like it would be enough to pass the expected values to runURLBarSearchTest, that would make the test easier to follow. @@ +10,5 @@ > +registerCleanupFunction(function() { > + Services.prefs.clearUserPref("browser.fixup.domainwhitelist.localhost"); > +}); > + > +function waitForNotification(notificationBox) { I think you might simplify this be re-using waitForCondition in the case we expect a notification, it is currently reinventing the polling that waitForCondition already implements. function promiseNotificationForTab(value, expected, tab=gBrowser.selectedTab) { let deferred = Promise.defer(); let notificationBox = gBrowser.getNotificationBox(tab.linkedBrowser); if (expected) { waitForCondition(() => notificationBox.getNotificationWithValue(value) !== null, deferred.resolve, "Were expecting to get a notification"); } else { setTimeout(() => { is(notificationBox.getNotificationWithValue(value), null, "We are expecting to not get a notification"); deferred.resolve(); }, 1000); } return deferred; } In future it could be shared in head.js @@ +32,5 @@ > + }, 100); > + return deferred.promise; > +} > + > +function* runURLBarSearchTest() { value, expectSearch and expectNotification should be arguments of this generator (or a single options object with such named properties) @@ +34,5 @@ > +} > + > +function* runURLBarSearchTest() { > + let tab = gBrowser.addTab(); > + gBrowser.selectedTab = tab; let tab = gBrowser.selectedTab = gBrowser.addTab(); @@ +58,5 @@ > + valueToOpen = "www.mozilla.org"; > + expectSearch = false; > + expectNotification = false; > + yield* runURLBarSearchTest(); > + gBrowser.removeTab(gBrowser.selectedTab); since you remove the tab in the test, I'd prefer if you'd also add it in the test. As it is looks like you are removing a tab nobody added (cause runURLBarSearchTest added it secretly) please just move let tab = gBrowser.selectedTab = gBrowser.addTab(); here and make runURLBarSearchTest use gBrowser.selectedTab. @@ +79,5 @@ > + let pref = "browser.fixup.domainwhitelist.localhost"; > + let prefValue = false; > + try { > + prefValue = Services.prefs.getBoolPref(pref); > + } catch (ex) {} it's a default pref, how can this throw?

(In reply to Justin Dolske [:Dolske] from comment #69) > Mak: also, if there's more work needed after a review pass, let me know and > I'll find someone to help finish it. Provided it would be fine for me to address my own comments, I might update the patches and push them. Otherwise anyone could do the same.

fwiw, I'm fixing the patches and will push them to try before proceeding.

The patches are updated and locally the test is passing and the functionality looks good. Considered we want this in 33 I'm going to land it.

https://hg.mozilla.org/integration/fx-team/rev/17a0436f86ee https://hg.mozilla.org/integration/fx-team/rev/e92d6fb6dd5f https://hg.mozilla.org/integration/fx-team/rev/16ef256da46b

https://hg.mozilla.org/mozilla-central/rev/17a0436f86ee https://hg.mozilla.org/mozilla-central/rev/e92d6fb6dd5f https://hg.mozilla.org/mozilla-central/rev/16ef256da46b

Someone forgot to remove a keyword :)

Hi Florin, can you assign a QA contact for verification of this bug.

I tested the fix for this issue on Nightly 33.0a1 (2014-07-20), Windows 7 64-bit, using various input. Detailed test results can be found in this etherpad [1]. Here are a few concerns on this matter: 1) using 't:' as input (w/o apostrophe) points to a "File not found" server error page, as the user is redirected to file:///t:/ URL 2) using 'test.' as input (w/o apostrophe) points to a "Server not found" error page, as the user is redirected to www.test. URL 3) using 'a\' as input (w/o apostrophe) results in a google search for a/ instead 4) using '0' as input (w/o apostrophe) points the user to an "Unable to connect" server error page, while using '1' as input (w/o apostrophe) results in a google search for the 1 keyword 5) using '192.168.0.1' as input (w/o apostrophe) points to a "Server not found" error page Gijs, could you please take a look at these and confirm whether they meet their expected behavior or not? [1] https://etherpad.mozilla.org/Bug693808

(In reply to Andrei Vaida, QA [:avaida] from comment #82) > I tested the fix for this issue on Nightly 33.0a1 (2014-07-20), Windows 7 > 64-bit, using various input. Detailed test results can be found in this > etherpad [1]. > > Here are a few concerns on this matter: > 1) using 't:' as input (w/o apostrophe) points to a "File not found" server > error page, as the user is redirected to file:///t:/ URL I know why this is, but it's probably not correct from a user perspective. This is tricky though. Under what circumstances would we fix this up? Needinfo'ing Boris for this question. Basically, we turn input of the form "X:\foo\bar\baz" into a file URL on Windows. While I guess "C:" should work (in that it shows you file:///C:/), if you have no drive at "T" then "T:" should send you to the search provider. However, the probability of someone using a single-letter search term followed by a colon and nothing else in order to do a search seems very low. Making all not-found files go to the search provider could result in leaking private data (if I mistype C:\Documents and Settings\Gijs\mysercetlife.txt") so that might not be desired either. Therefore, I think we might be fine with the current behaviour, because I think the number of false negatives in detecting search queries now would be smaller (and have less negative impact) than the number of false positives if we made all not-found-files automatically do a search. Boris, can you confirm? > 2) using 'test.' as input (w/o apostrophe) points to a "Server not found" > error page, as the user is redirected to www.test. URL I know why this is, but it's probably also not correct from a user's perspective. Please file a bug about inputs ending in a '.' causing this behaviour. > 3) using 'a\' as input (w/o apostrophe) results in a google search for a/ > instead This is just a bug, period. Not sure why it happens. Please file a followup for this, too. > 4) using '0' as input (w/o apostrophe) points the user to an "Unable to > connect" server error page, while using '1' as input (w/o apostrophe) > results in a google search for the 1 keyword I can't reproduce this on mac... I wonder if it's specific to DNS on Windows... > 5) using '192.168.0.1' as input (w/o apostrophe) points to a "Server not > found" error page This seems correct assuming that server really didn't exist (ie there is no working http server on that address, and the result is the same if you input "http://192.168.0.1/" without quotes) - can you confirm that?

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #83) > (In reply to Andrei Vaida, QA [:avaida] from comment #82) > > I tested the fix for this issue on Nightly 33.0a1 (2014-07-20), Windows 7 > > 64-bit, using various input. Detailed test results can be found in this > > etherpad [1]. > > > > Here are a few concerns on this matter: > > 1) using 't:' as input (w/o apostrophe) points to a "File not found" server > > error page, as the user is redirected to file:///t:/ URL > > I know why this is, but it's probably not correct from a user perspective. > > This is tricky though. Under what circumstances would we fix this up? > Needinfo'ing Boriss for this question. Basically, we turn input of the form > "X:\foo\bar\baz" into a file URL on Windows. > > While I guess "C:" should work (in that it shows you file:///C:/), if you > have no drive at "T" then "T:" should send you to the search provider. > However, the probability of someone using a single-letter search term > followed by a colon and nothing else in order to do a search seems very low. > > Making all not-found files go to the search provider could result in leaking > private data (if I mistype C:\Documents and Settings\Gijs\mysercetlife.txt") > so that might not be desired either. > > Therefore, I think we might be fine with the current behaviour, because I > think the number of false negatives in detecting search queries now would be > smaller (and have less negative impact) than the number of false positives > if we made all not-found-files automatically do a search. Boriss, can you > confirm? Actually setting needinfo (and correcting nicknames...)

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #83) > > 4) using '0' as input (w/o apostrophe) points the user to an "Unable to > > connect" server error page, while using '1' as input (w/o apostrophe) > > results in a google search for the 1 keyword > > I can't reproduce this on mac... I wonder if it's specific to DNS on > Windows... This issue is actually reproducing intermittently on my Windows machine, so I'll have to investigate it further. > > 5) using '192.168.0.1' as input (w/o apostrophe) points to a "Server not > > found" error page > > This seems correct assuming that server really didn't exist (ie there is no > working http server on that address, and the result is the same if you input > "http://192.168.0.1/" without quotes) - can you confirm that? Using "http://192.168.0.1/" as input without the quotes has the same behavior. Thank you for the clarifications Gijs, I'll file follow-up bugs for issues 2), 3) and 4) as soon as I finish my investigation.

(In reply to Marco Bonardo [:mak] from comment #77) > The patches are updated and locally the test is passing and the > functionality looks good. Considered we want this in 33 I'm going to land > it. Thanks a lot for your review comments and for landing this in time for 33! Same for Dão's comments. Much appreciated. :-)

> Boris, can you confirm? Did you mean me or boriss? ;) > Please file a bug about inputs ending in a '.' causing this behaviour. Please be sure to not break things like "www.mozilla.com." which people who are paranoid enough about their current DNS search settings will type. > Not sure why it happens. Because on Windows we convert backslash to forwardslash when creating URIs from strings in various cases? On Mac this doesn't seem to happen.

(In reply to Boris Zbarsky [:bz] from comment #87) > > Boris, can you confirm? > > Did you mean me or boriss? ;) Boriss, see my later comment. Sorry, I got confused. :-) > > Please file a bug about inputs ending in a '.' causing this behaviour. > > Please be sure to not break things like "www.mozilla.com." which people who > are paranoid enough about their current DNS search settings will type. Oh, hm, interesting. Are there cases where this would not involve multiple dots? Your input has 3, I could imagine 2 ("mozilla.com.") - but 1? > > Not sure why it happens. > > Because on Windows we convert backslash to forwardslash when creating URIs > from strings in various cases? On Mac this doesn't seem to happen. I suspect so, yes, but the search query should be done using the original input, not the forward-slashed file case, I thought, and I didn't/don't have time to look at this again (bugmail-catchup-post-PTO-funtimes!). I suspect that that is exactly where the issue lies in the end, but I'd rather investigate in a separate bug than reopen this one just for that issue. :-)

Only one dot wouldn't give you a FQDN, just a fully qualified TLD, so I think the one-dot case is ok to munge.

(In reply to Andrei Vaida, QA [:avaida] from comment #85) > (In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #83) > > > 4) using '0' as input (w/o apostrophe) points the user to an "Unable to > > > connect" server error page, while using '1' as input (w/o apostrophe) > > > results in a google search for the 1 keyword > > > > I can't reproduce this on mac... I wonder if it's specific to DNS on > > Windows... > This issue is actually reproducing intermittently on my Windows machine, so > I'll have to investigate it further. > > > > 5) using '192.168.0.1' as input (w/o apostrophe) points to a "Server not > > > found" error page > > > > This seems correct assuming that server really didn't exist (ie there is no > > working http server on that address, and the result is the same if you input > > "http://192.168.0.1/" without quotes) - can you confirm that? > Using "http://192.168.0.1/" as input without the quotes has the same > behavior. > > Thank you for the clarifications Gijs, I'll file follow-up bugs for issues > 2), 3) and 4) as soon as I finish my investigation. Sounds great! Needinfo'ing to make sure we don't forget to get the followup bugs done - I may still have time to work on them before my PTO (or otherwise shortly afterwards)

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #90) > Sounds great! Needinfo'ing to make sure we don't forget to get the followup > bugs done - I may still have time to work on them before my PTO (or > otherwise shortly afterwards) Thanks for the reminder! :) I filed Bug 1042519 for issue 2) and Bug 1042521 for issue 3). At this point I'm unable to determine precise STR for issue 4), I suspect it might have been isolated to my session/profile. Please let me know if there's anything else I can help with here.

(In reply to Andrei Vaida, QA [:avaida] from comment #91) > (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #90) > > Sounds great! Needinfo'ing to make sure we don't forget to get the followup > > bugs done - I may still have time to work on them before my PTO (or > > otherwise shortly afterwards) > Thanks for the reminder! :) > > I filed Bug 1042519 for issue 2) and Bug 1042521 for issue 3). At this point > I'm unable to determine precise STR for issue 4), I suspect it might have > been isolated to my session/profile. > > Please let me know if there's anything else I can help with here. I think we're all good - except, I'm guessing this means we can mark this verified on 33 and 34? :-)

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #92) > I think we're all good - except, I'm guessing this means we can mark this > verified on 33 and 34? :-) Yes, this is now verified fixed.

Added in the 33 release notes with the wording "Improved search experience through the location bar" Hoping to have a blog post to point to.

(In reply to Sylvestre Ledru [:sylvestre] from comment #95) > Added in the 33 release notes with the wording "Improved search experience > through the location bar" > Hoping to have a blog post to point to. I blogged about it here, http://msujaws.wordpress.com/2014/08/01/faster-and-snappier-searches-now-in-firefox-aurora/

Thanks. Updated!

Redirecting the question based on comment 82/83 to Philipp: > (In reply to Andrei Vaida, QA [:avaida] from comment #82) > > I tested the fix for this issue on Nightly 33.0a1 (2014-07-20), Windows 7 > > 64-bit, using various input. Detailed test results can be found in this > > etherpad [1]. > > > > Here are a few concerns on this matter: > > 1) using 't:' as input (w/o apostrophe) points to a "File not found" server > > error page, as the user is redirected to file:///t:/ URL > > I know why this is, but it's probably not correct from a user perspective. > > This is tricky though. Under what circumstances would we fix this up? > Needinfo'ing <<Philipp>> for this question. Basically, we turn input of the form > "X:\foo\bar\baz" into a file URL on Windows. > > While I guess "C:" should work (in that it shows you file:///C:/), if you > have no drive at "T" then "T:" should send you to the search provider. > However, the probability of someone using a single-letter search term > followed by a colon and nothing else in order to do a search seems very low. > > Making all not-found files go to the search provider could result in leaking > private data (if I mistype C:\Documents and Settings\Gijs\mysercetlife.txt") > so that might not be desired either. > > Therefore, I think we might be fine with the current behaviour, because I > think the number of false negatives in detecting search queries now would be > smaller (and have less negative impact) than the number of false positives > if we made all not-found-files automatically do a search. Philipp, can you > confirm?

(In reply to :Gijs Kruitbosch from comment #102) > Redirecting the question based on comment 82/83 to Philipp: > > > (In reply to Andrei Vaida, QA [:avaida] from comment #82) > > > I tested the fix for this issue on Nightly 33.0a1 (2014-07-20), Windows 7 > > > 64-bit, using various input. Detailed test results can be found in this > > > etherpad [1]. > > > > > > Here are a few concerns on this matter: > > > 1) using 't:' as input (w/o apostrophe) points to a "File not found" server > > > error page, as the user is redirected to file:///t:/ URL > > > > I know why this is, but it's probably not correct from a user perspective. > > > > This is tricky though. Under what circumstances would we fix this up? > > Needinfo'ing <<Philipp>> for this question. Basically, we turn input of the form > > "X:\foo\bar\baz" into a file URL on Windows. > > > > While I guess "C:" should work (in that it shows you file:///C:/), if you > > have no drive at "T" then "T:" should send you to the search provider. > > However, the probability of someone using a single-letter search term > > followed by a colon and nothing else in order to do a search seems very low. > > > > Making all not-found files go to the search provider could result in leaking > > private data (if I mistype C:\Documents and Settings\Gijs\mysercetlife.txt") > > so that might not be desired either. > > > > Therefore, I think we might be fine with the current behaviour, because I > > think the number of false negatives in detecting search queries now would be > > smaller (and have less negative impact) than the number of false positives > > if we made all not-found-files automatically do a search. Philipp, can you > > confirm? Sounds like a fair trade-off. Let's keep it that way.

This change came just in time for .gmail becoming a TLD that actually gives you an answer. Right now writing "gmail" on Firefox 32's URL box and hitting enter returns an "Unable to connect" page instead of a search page like Aurora does.

(In reply to cousteau from comment #106) > This change came just in time for .gmail becoming a TLD that actually gives > you an answer. Right now writing "gmail" on Firefox 32's URL box and > hitting enter returns an "Unable to connect" page instead of a search page > like Aurora does. I'm not sure what you're trying to say here. Let me lay out the state of things, as I understand them, and maybe you can point out what you think is wrong about this patch or why this is relevant. Firefox 32 (release) doesn't have this patch, while 33 (beta) and later does. gmail being a tld doesn't mean anything for putting just "gmail" in your location bar, which (assuming no setup default TLDs) would try to connect to "http://gmail./" which isn't in the ".gmail" TLD. http://.gmail/, by itself, just like http://.com/, doesn't work, either, so showing a search page if you don't explicitly specify the http:// bit seems correct. Actually using "something.gmail" won't trigger the logic that was implemented in this bug, so should be unaffected as well.

(In reply to :Gijs Kruitbosch from comment #107) > gmail being a tld doesn't mean anything for putting just "gmail" in your > location bar, which (assuming no setup default TLDs) would try to connect to > "http://gmail./" which isn't in the ".gmail" TLD. In that case I don't understand why writing "gmail" on the URL bar and hitting enter shows an "Unable to connect" page (in Firefox 32) rather than using the default search engine to search for "gmail" as happens with, say, "mozilla". I thought it was related to gmail being registered as a TLD and the way it handles connections (`nslookup gmail` returns 127.0.53.53 rather than just a NXDOMAIN or No Answer as `nslookup mozilla` and `nslookup com` do). In any case, this patch seems to solve this issue as this only happens with Firefox 32 and not with Firefox Aurora, where the patch is applied.

(In reply to cousteau from comment #108) > (In reply to :Gijs Kruitbosch from comment #107) > > gmail being a tld doesn't mean anything for putting just "gmail" in your > > location bar, which (assuming no setup default TLDs) would try to connect to > > "http://gmail./" which isn't in the ".gmail" TLD. > > In that case I don't understand why writing "gmail" on the URL bar and > hitting enter shows an "Unable to connect" page (in Firefox 32) rather than > using the default search engine to search for "gmail" as happens with, say, > "mozilla". I thought it was related to gmail being registered as a TLD and > the way it handles connections (`nslookup gmail` returns 127.0.53.53 rather > than just a NXDOMAIN or No Answer as `nslookup mozilla` and `nslookup com` > do). Ah, so I was partially right, and partially wrong: http://stackoverflow.com/questions/25662590/domain-foo-bar-points-127-0-53-53-why explains why you see that IP, which then doesn't connect and shows you the error page. Just "mozilla" doesn't resolve, which means we go to search in Firefox 32 - but only once DNS failure has happened, which is very slow for some people. This patch made it so we go to search immediately for single-word input with no dots etc. (but provide you a notification bar if you want to whitelist a single-word host, and have "localhost" whitelisted by default) > In any case, this patch seems to solve this issue as this only happens with > Firefox 32 and not with Firefox Aurora, where the patch is applied. :-)

There should be an option to overwrite this behavior. This change modifies the behavior when comparing to the previous version of firefox, and the user should be able to have an option to revert to the original behavior. Usage example: In environment with hundreds of single word/keyword domains( example server01, server02 …), adding all of them to the white-list is, at the very least, difficult. User should be able to disable the “search and display results first, if DNS revolution succeeds suggest the URL” and revert back to “wait for DNS resolution, and only if it fails display search results”

(In reply to insiac from comment #110) > There should be an option to overwrite this behavior. This change modifies > the behavior when comparing to the previous version of firefox, and the user > should be able to have an option to revert to the original behavior. > Usage example: In environment with hundreds of single word/keyword domains( > example server01, server02 …), adding all of them to the white-list is, at > the very least, difficult. > > User should be able to disable the “search and display results first, if DNS > revolution succeeds suggest the URL” and revert back to “wait for DNS > resolution, and only if it fails display search results” Please file a separate bug for this request.

I am hoping that this is the correct and helpful place to post this information. If it is not, please kindly provide the appropriate place. The behavior of Firefox (tested on v39) changes depending on the DNS which the host system is currently using. Since the DNS cannot be modified within Firefox, this leads to different experiences at different times, as well as different experiences for different Firefox users. Further confusing the user, there is no indication to them why Firefox sometimes behaves differently or any instructions on how to make it behave the same across all systems. When using some DNS servers, entering 'abcd' (without the single quotes) into the Location Bar results in a search being perfomed for 'abcd'. This looks great. When using other DNS servers, performing the identical search results in a confusing notification asking 'Did you mean to go to abcd?', followed by a search. Toggling browser.fixup.alternate.enabled has no effect on whether or not this notification sometimes appears. We should not be presenting a different experience to the user depending on the DNS server being used. Consistency is key, especially in the day and age of browser add-on malware. As we all know, there is a fair amount of malware that tries to redirect the user to another website. I recommend changing the user interface so that it is consistent, regardless of the DNS server currently used.

(In reply to quality+bugzilla from comment #112) > I am hoping that this is the correct and helpful place to post this > information. If it is not, please kindly provide the appropriate place. Please file a new report, thank you.

Thank you. I created Bug #1180329 per your request.