Andrei Vaida [:avaida] Reporter
	Description • 10 years ago

Note: this is a follow-up for Bug 693808.

Reproducible on Nightly 34.0a1 (2014-07-22), Build ID: 20140722030201, using:
 * Windows 7 64-bit,
 * Ubuntu 14.04 LTS 32-bit,
 * Mac OS X 10.9.4.

Steps to reproduce:
1. Launch Nightly 34.
2. Bring the Location Bar into focus and type in 'test.' without the apostrophe.
3. Press the <enter> key.

Expected result: 
A google search is performed for the 'test.' keyword (w/o apostrophe).

Actual result: 
The user is redirected to a "Server not found" error page, with www.test. as URL.

	:Gijs (he/him) Assignee
	Comment 1 • 10 years ago

I removed a bunch of logic that the test mirrored from the implementation by just adding the info to the test - the test was getting hard to maintain. I also noticed I missed a case where the info object was not correctly updated with information about whether or not a dot was present.

	:Gijs (he/him) Assignee
	Comment 2 • 10 years ago

Jenn, can you add this one, too?

	:Gijs (he/him) Assignee
	Comment 3 • 10 years ago

Note that inputHostHasDot is becoming even more fuzzy now, because it's false for the "test." case. Which I am somewhat uneasy about, but I can't think of a better name. :-(

	Marco Mucci [:MarcoM]
	Comment 4 • 10 years ago

Added to Iteration 34.1

	:Gijs (he/him) Assignee
	Comment 5 • 10 years ago

Comment on attachment 8462094 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Actually, no, I should hold off on asking for review here.

I think I'm suffering from "let's design an API without any actual consumers" issues. It's not clear to me what consumers generally would want to know about the input here. It approximates "is it even possible for this input to end up approximately being a URI" and/or "did we only need a protocol to fix this up, or did we need to do more, or did we go the whole hog and just keyword-search it?" - but currently only the "fixupUsedKeyword" bool is in use by the browser.js consumer, and that's written from the "what did we do to get this output" perspective, not "what was the input like".

I'm half-tempted to just remove the inputHostHasDot bool (and probably the inputHasProtocol one as well) from the info object altogether, or to rewrite them as "what did we to to get this output (in other words, add "fixupAddedProtocol" and "fixupCreatedAlternativeURI" flags instead).

Which kind of sucks because we just landed this and it's now on Aurora... although I suppose that also means "better sooner than later"...

Thoughts?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

You could do that and uplift to Aurora?

	:Gijs (he/him) Assignee
	Comment 7 • 10 years ago

OK. I'm much happier with this than the previous patch. :-)

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

Comment on attachment 8462489 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

>+    uint32_t dotCount = aURIString.CountChar('.');

Might be faster for long URIs to RFindChar and then see whether it's found at dotLoc?  Unclear.  I'm probably OK with it either way.

>+            if (dotLoc == aURIString.Length()) {

How could that happen?  Missing a -1?

r=me with the above dealt with.

I didn't read the test changes very carefully.  Let me know if I should.

	:Gijs (he/him) Assignee
	Comment 9 • 10 years ago

Comment on attachment 8462489 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Review of attachment 8462489 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDefaultURIFixup.cpp
@@ +967,5 @@
>              nsAutoCString pref("browser.fixup.domainwhitelist.");
> +            if (dotLoc == aURIString.Length()) {
> +              pref.Append(Substring(asciiHost, 0, asciiHost.Length() - 1));
> +            } else if (dotLoc == 0) {
> +              pref.Append(StringTail(asciiHost, asciiHost.Length() - 1));

Adding a test for ".localhost" in browser code showed this doesn't actually work: if not doing a search lookup, ".localhost" is not equivalent to "localhost", and the URI fixup code changes "http://.localhost/" to "http://www..localhost/" which is a different host still. So I've removed this "else if" block again so ".localhost" is controlled by a different whitelist pref than "localhost".

However, I've kept the code ~12 lines up so as to do a search query for ".localhost" by default (which it didn't do before). This is what IE does. Chrome corrects to "localhost" and loads that. If someone feels strongly that we should put work into making that syntax work, I think that should be a separate followup bug.

In the meantime, I don't think this change warrants re-requesting review on the core bits... but we need a browser part, so I'll request review for a new patch from a browser peer in a bit.

	:Gijs (he/him) Assignee
	Comment 10 • 10 years ago

I've adjusted the things bz found in his review, but when testing the length - 1 pref issue, I noticed that the frontend code also needs to be adapted slightly to get the whitelisting right. r?jaws on the browser bits (I added a test for this case).

	:Gijs (he/him) Assignee
	Comment 11 • 10 years ago

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Marco, seems Jared might not be here... do you have time to look at the browser/ part here? It's quite small... :-)

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 12 • 10 years ago

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Review of attachment 8466979 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +795,5 @@
>    let hostName = alternativeURI.host;
>    // and the ascii-only host for the pref:
>    let asciiHost = alternativeURI.asciiHost;
> +  // Normalize out the single trailing dot:
> +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {

asciiHost.endsWith('.') (fwiw here you should have used lastIndexOf)

@@ +796,5 @@
>    // and the ascii-only host for the pref:
>    let asciiHost = alternativeURI.asciiHost;
> +  // Normalize out the single trailing dot:
> +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> +    asciiHost = asciiHost.substring(0, asciiHost.length - 1);

asciiHost = asciiHost.slice(0, -1);

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +49,5 @@
>  });
>  
> +function get_test_function_for_localhost_with_hostname(hostName) {
> +  return function* test_navigate_single_host() {
> +    let pref = "browser.fixup.domainwhitelist.localhost";

nit: const

	:Gijs (he/him) Assignee
	Comment 13 • 10 years ago

(In reply to Marco Bonardo [:mak] from comment #12)
> Comment on attachment 8466979 [details] [diff] [review]
> test. should result in a keyword lookup instead of an error page,
> 
> Review of attachment 8466979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +795,5 @@
> >    let hostName = alternativeURI.host;
> >    // and the ascii-only host for the pref:
> >    let asciiHost = alternativeURI.asciiHost;
> > +  // Normalize out the single trailing dot:
> > +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> 
> asciiHost.endsWith('.') (fwiw here you should have used lastIndexOf)

Commented on why that wasn't an option.

> @@ +796,5 @@
> >    // and the ascii-only host for the pref:
> >    let asciiHost = alternativeURI.asciiHost;
> > +  // Normalize out the single trailing dot:
> > +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> > +    asciiHost = asciiHost.substring(0, asciiHost.length - 1);
> 
> asciiHost = asciiHost.slice(0, -1);
> 
> :::
> browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.
> js
> @@ +49,5 @@
> >  });
> >  
> > +function get_test_function_for_localhost_with_hostname(hostName) {
> > +  return function* test_navigate_single_host() {
> > +    let pref = "browser.fixup.domainwhitelist.localhost";
> 
> nit: const

Addressed these.

remote:   https://hg.mozilla.org/integration/fx-team/rev/782db4943ae5

	Ed Morley [:emorley]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/782db4943ae5

	Andrei Vaida [:avaida] Reporter
	Comment 15 • 10 years ago

Verified fixed on Nightly 34.0a1 (2014-08-06) using Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.4. Leaving the [qa+] keyword in place until this fix reaches Aurora.

	:Gijs (he/him) Assignee
	Comment 16 • 10 years ago

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Approval Request Comment
[Feature/regressing bug #]: 693808
[User impact if declined]: different APIs on Aurora/nightly for urifixup which might confuse add-ons, some user input might not correctly trigger searches
[Describe test coverage new/current, TBPL]: has automated tests, has been verified by QA
[Risks and why]: low-to-medium risk - low because the change shouldn't really affect things and the code is gaining code coverage to the point where I would say it's pretty good now, but medium-ish because this affects a really oft-used feature in the browser (the location bar!) and so... yeah.
[String/UUID change made/needed]: 1 uuid change, should be good for Aurora AIUI.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c111b7cf29ff

	Andrei Vaida [:avaida] Reporter
	Comment 18 • 10 years ago

Verified fixed on Aurora 33.0a2 (2014-08-08) as well, using Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.4.