621274 - Location Bar remembers invalid (Server not found) URLs

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Christian Ascheberg]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b9pre) Gecko/20101223 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b9pre) Gecko/20101223 Firefox/4.0b9pre

See STR below.
This is a regression:
works:
win32 2010-12-15-03-mozilla-central f11f7ed625ba
broken:
win32 2010-12-16-03-mozilla-central a5413c3c1013

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f11f7ed625ba&tochange=a5413c3c1013

Reproducible: Always

Steps to Reproduce:
1. visit for example http://www,google.com (note the comma instead of dot)
2. firefox will redirect to http://www.www,google.com/ and show Server not found error page
3. type google into location bar
Actual Results:  
http://www.www,google.com/ will show up as a result.

Expected Results:  
I think this should not show up.

Comment 1

[Christian Ascheberg]

I wonder why firefox is actually trying to fix the URL like this instead of showing a more specific error page.

Comment 2

[Alice0775 White]

Confirmed.

Works:
http://hg.mozilla.org/mozilla-central/rev/e435c9812855
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215
Firefox/4.0b9pre ID:20101215174043
Fails:
http://hg.mozilla.org/mozilla-central/rev/cd62a8afc52e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215
Firefox/4.0b9pre ID:20101215204205
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e435c9812855&tochange=cd62a8afc52e

Comment 3

[Alice0775 White]

Linux too
http://hg.mozilla.org/mozilla-central/rev/ef42c524718f
Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20101223 Firefox/4.0b9pre ID:20101223030401

Comment 4

[Alice0775 White]

Please ignore comment #3,
I can not reproduce on Linux

Comment 5

[Alice0775 White]

In local build
Build from af7e6f7cbf16 : fails
Build from 2a7a3641d67d : fails
Build from 6617dcb0d48a : fails
Build from d65c65ef13a4 : works
Build from 20311c05703e : works
Build from b198eca4d183 : works
Build from 4626f19fa27e : works

So, this is regressen by Bug 599973 - Don't use steps for async favicons

Comment 6

[Alice0775 White]

And also
Build from 999385f355a3 : fails

Comment 7

[Marco Bonardo [:mak]]

I have to check what we were doing before, but most likely part of the problem is in  "INSERT INTO moz_places (url, rev_host, favicon_id) ", where we don't set hidden = 1, I thought that one was the default value but looks like we default to 0 :(
Still, we should not try to add invalid pages since I'd guess we fail to gather a icon from network (onStopRequest) and we use the error icon.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Apart fixing icons service to avoid storing error icons and marking pages as hidden by default, I changed feedWriter use of history and favicons services, because it was verbose and not efficient.
The new code is simpler, kills a useless history observer and a bunch of main-thread IO. It still passes through favicon service and saves icons for handlers, we could maybe avoid it, but first of all I want to avoid privacy/security regressions (I practically replicated code path as it was) and second it just happens in feed previews so it doesn't matter much globally.
This components needs more cleanup but post FX4.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 503024 [details] [diff] [review]
patch v1.0

>+++ b/toolkit/components/places/tests/head_common.js
>+/**
>+ * Returns the hidden status of a url.
>+ *
>+ * @param  aURI
>+ *         The URI or spec to get hidden for.
nit: two spaces after @param; should only be one

>+ * @return the hidden value.
nit: @return true if the url is hidden, false otherwise.

>+++ b/toolkit/components/places/tests/unit/test_doSetAndLoadFaviconForPage_failures.js
>+    desc: "test setAndLoadFaviconForPage with error icon",
>+    pageURI: uri("http://test4.bar/"),
>+    go: function go4() {
>+
>+      PlacesUtils.favicons.setAndLoadFaviconForPage(
>+        this.pageURI, NetUtil.newURI(FAVICON_ERRORPAGE_URL), true
>+      );
>+    },
You are using uri() and NetUtil.newURI.  I'd prefer the latter.

>   { // This is a valid icon set test, that will cause the closing notification.
>     desc: "test setAndLoadFaviconForPage for valid history uri",
>-    pageURI: uri("http://test4.bar/"),
>-    go: function go4() {
>+    pageURI: uri("http://testfinal.bar/"),
ditto

>     // Ensure we have been called by the last test.
>-    do_check_true(pageURI.equals(uri("http://test4.bar/")));
>+    do_check_true(pageURI.equals(uri("http://testfinal.bar/")));
ditto

r=sdwilsh

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/1c1216dcafed

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/1c1216dcafed

Comment 14

[u279076]

Verified with Firefox 4.0b10.

Note: Added an extra step to the original STR...

Original STR:
1. Visit www,google.com
2. Firefox loads http://www.www,google.com/ and shows Server not
found error page
3. Type "google" into the location bar

My STR:
1. Visit www,google.com
2. Firefox loads http://www.www,google.com/ and shows Server not
found error page
3. Click HOME
4. Type "google" into the location bar

Without the addition of clicking the HOME button, this will load a switch-to-tab result for http://www.www,google.com/ because it is still an active tab.  I believe this is expected.