I have visited pages on "dd-wrt.com" on both the http and ftp protocols. Whenever I type in "www.dd" into the location bar, it suggests "www.dd-wrt.com", and autofills that into the location bar. When I press Enter to confirm the suggestion, it takes me to "ftp://dd-wrt.com/" instead of "http://www.dd-wrt.com/" the http site.

ftp gets priority and "www." prefix is stripped from the search. We could likely improve it somehow, though doing the opposite would break the ftp://www.domain case. We should fix it though, since it's unclear where you are going from the suggestion, maybe exactly making ftp the last choice, and if a domain supports both the user will have to type the ftp:// prefix. As a workaround you may type the protocol for now, does that work?

Additional information from duplicate bug: even when typing in http://url.of.website, it will still load the ftp variant. In my instance, I can not remember visiting ftp://gcc.gnu.org, but I may have done so in the past.

I get this same bug with my router, which I have visited as https://192.168.0.1/ When I type http://192.168.0.1 (no trailing slash) it autocompletes to https://192.168.0.1/, meaning it replaces my complete URL with one of a different protocol.

taking to have it on the radar

comment 0 will be fixed by bug 781617 as well as other similar cases. At least will be possible to override the choice by typing the protocol you wish. What's left to decide is whether we want to completely drop ftp protocol prefix or change the rule so that ftp is suggested only if it's the only protocol (may be slow though).

comment 4 should be fixed by that bug as well since it's typing the protocol explicitly.

Can any of the reporters here comment whether the current Nightly behavior is acceptable for you issue?

Since this doesn't involve security the resolution could be just to use ftp only if all of the known pages in the specific host use ftp.

This does a couple things: 1. error pages are no more marked as typed, this avoids them to be considered for autofill (part of it really) 2. For ftp:// and www. prefixes instead of checking if any page exists with those prefixes, it rather checks if all typed pages have it The global effect should be that only hosts that are ftp only are suggested as ftp, as well as hosts that have both www. and not are suggested without a prefix. Thanks to the previous fixes to autocomplete this basically allows the user to enforce ftp or www where wanted, while in common cases we'll still suggest the right prefix. I didn't touch https:// and https://www. prefixes at all here, for the obvious security issues we discussed, that should be done apart (bug 840027). I'm pushing to Try now, since I could ran only part of the tests locally. Finally, after looking again at complication of these queries, I'm serously thinking we should drop www. from the rev_host column, it is causing trouble to us (code complication and performance) and to the users (grouping by site shows both the www and the non-www hosts), the benefit is just that in an ideal world the 2 may point to different stuff, but we ignore that in so many cases already for history purposes... I think I'll file a bug to evaluate doing that.

filed bug 843357 for "www." in rev_host

https://tbpl.mozilla.org/?tree=Try&rev=8fe1323d1e5c all green but xpcshell, but looks like I got an unlucky changeset from inbound there. Will repush an xpcshell-only build.

I have verified the latest nightly build (2013-02-21). I have both HTTP and FTP versions of gcc.gnu.org and the FTP version is the autocomplete target. The current behavior is as follows: 1) if I type 'gcc' and press Enter, I'm autocompleted to the FTP version of gcc.gnu.org, 2) if I type http://gcc and press Enter, the autocomplete correctly brings me to the HTTP version (so comment #3 no longer applies for me). For clarity, this didn't work in FF 19, 3) if I delete the FTP site from the autocomplete suggestions and I type 'gcc' in the same tab or new tab, I don't have an FTP site in the list of suggestions but still I get the FTP site when pressing Enter. So for me the only remaining problem is that I'd like to change the autocomplete behavior to the HTTP version and there is no way to do this, as point 3) above doesn't work. (I'm not sure whether it is supposed to though).

Comment on attachment 716242 [details] [diff] [review] patch v1.0 Review of attachment 716242 [details] [diff] [review]: ----------------------------------------------------------------- I've provided some basic feedback, and tried the patch locally where it seems to work correctly. The test changes also look good. A formal review might require some more time, since I don't know this area of the code. You might also want the opinion of someone more familiar with this area, but feel free to re-request review if needed. ::: toolkit/components/places/Database.cpp @@ +1859,5 @@ > > // Update prefixes. > nsCOMPtr<mozIStorageAsyncStatement> updatePrefixesStmt; > rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( > "UPDATE moz_hosts SET prefix = ( " Do we need to force this migration step again, or is it already executed? ::: toolkit/components/places/nsPlacesTriggers.h @@ +53,5 @@ > * unprefixed ones. > + * The secure conditions just check if a page exists for a specific prefix, > + * while the ftp and www prefixes are returned only if all the pages for the > + * given host have them. > + * The searches are limited to typed pages. The logic has become a bit difficult to understand with this addition, I'd just rewrite the comment from scratch and include a bullet list: - If at least one page... we return https - If... we return ftp - ... I'd also document "_rev_host_predicate" independently as a parameter. @@ +72,4 @@ > ") THEN 'ftp://' " \ > + "WHEN 1 = ( " \ > + "SELECT avg(substr(url,1,11) = 'http://www.') = 1 FROM moz_places h " \ > + "WHERE (" _rev_host_predicate ") AND +h.typed = 1 " \ I have to assume that this "avg" trick is the best syntax SQLite offers for what you're trying to do here? ::: toolkit/components/places/tests/unit/test_hosts_triggers.js @@ +198,5 @@ > > + yield promiseAddVisits([{ uri: TEST_URI_1, transition: TRANSITION_TYPED }, > + { uri: TEST_URI_2, transition: TRANSITION_TYPED }]); > +dump_table("moz_hosts"); > +dump_table("moz_places"); These look like debug leftovers.

(In reply to Paolo Amadini [:paolo] from comment #15) > A formal review might require some more time, since I don't know this area > of the code. You might also want the opinion of someone more familiar with > this area, but feel free to re-request review if needed. Unfortunately I'm lacking expert reviewers for these database changes, Gavin reviewed the first implementation but we can't drop any review on him :) Feel free to ask questions, I'll try to explain anything unclear. > ::: toolkit/components/places/Database.cpp > @@ +1859,5 @@ > > > > // Update prefixes. > > nsCOMPtr<mozIStorageAsyncStatement> updatePrefixesStmt; > > rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( > > "UPDATE moz_hosts SET prefix = ( " > > Do we need to force this migration step again, or is it already executed? My first approach was "let the db update incrementally while browsing", but in the end it's probably better to do a migration, so I will add one. > @@ +72,4 @@ > > ") THEN 'ftp://' " \ > > + "WHEN 1 = ( " \ > > + "SELECT avg(substr(url,1,11) = 'http://www.') = 1 FROM moz_places h " \ > > + "WHERE (" _rev_host_predicate ") AND +h.typed = 1 " \ > > I have to assume that this "avg" trick is the best syntax SQLite offers for > what you're trying to do here? I tried various combinations and this one ended up being the faster I could find. I don't exclude someone may find a better solution, I couldn't for now.

addressed previous comments

...but I forgot the dump_table stuff...

Comment on attachment 719474 [details] [diff] [review] patch v1.2 Review of attachment 719474 [details] [diff] [review]: ----------------------------------------------------------------- First pass... mostly questions. Unfortunately, this code area still looks like madness to me :-( ::: toolkit/components/places/nsPlacesTriggers.h @@ +50,5 @@ > * Select the best prefix for a host, based on existing pages registered for it. > * Prefixes have a priority, from the top to the bottom, so that secure pages > * have higher priority, and more generically "www." prefixed hosts come before > * unprefixed ones. > + * Given a host, examine associated pages and: I made these additions, do I understand correctly? * [...] unprefixed ones. * * Existing pages are found based on a variable named "host", that can match the * "host" column of the "hosts" table directly, or correspond to another * expression in the SQL statement. * * Given a host, examine [...] * * @param _rev_host_predicate * A SQL expression used to select which records of the moz_places table * are relevant to the sought host. This is generally obtained by * matching the rev_host column against the "host" variable, but rev_host * can be matched against another expression for efficiency, if the host * name is already available in reversed form. @@ +71,5 @@ > "SELECT 1 FROM moz_places WHERE url BETWEEN 'https://' || host || '/' " \ > "AND 'https://' || host || '/' || X'FFFF' " \ > ") THEN 'https://' " \ > + "WHEN 1 = ( " \ > + "SELECT avg(substr(url,1,4) = 'ftp:') = 1 FROM moz_places h " \ "SELECT min(...) FROM" seems to work as well and looks clearer, is it slower? Also, "= 1" seems redundant since we have "1 = (" in front of the SELECT and it's clear that we have a boolean expression inside "min()". @@ +72,5 @@ > "AND 'https://' || host || '/' || X'FFFF' " \ > ") THEN 'https://' " \ > + "WHEN 1 = ( " \ > + "SELECT avg(substr(url,1,4) = 'ftp:') = 1 FROM moz_places h " \ > + "WHERE (" _rev_host_predicate ") AND +h.typed = 1 " \ "The unary operator + is a no-op. It can be applied to strings, numbers, blobs or NULL and it always returns a result with the same value as the operand." @@ +94,5 @@ > "(SELECT id FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), " \ > "fixup_url(get_unreversed_host(NEW.rev_host)), " \ > "MAX(IFNULL((SELECT frecency FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), -1), NEW.frecency), " \ > "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), 0), NEW.typed), " \ > + "(" HOSTS_PREFIX_PRIORITY_FRAGMENT("rev_host = NEW.rev_host") \ How does this expression relate to considering existing pages in the same group regardless of the presence of the "www." prefix in the host name? Is that logic unneeded here, or may the trigger update the "hosts" table with a result that would be inconsistent with the one given if (for illustration purposes) we ran the migration step again?

(In reply to Paolo Amadini [:paolo] from comment #19) > * Existing pages are found based on a variable named "host", that can match > the > * "host" column of the "hosts" table directly, or correspond to another > * expression in the SQL statement. there are no variables in SQLite, it should just be the moz_hosts.host column here, what made you think it's a variable? > * @param _rev_host_predicate > * A SQL expression used to select which records of the moz_places > table > * are relevant to the sought host. This is generally obtained by > * matching the rev_host column against the "host" variable, but > rev_host > * can be matched against another expression for efficiency, if the > host > * name is already available in reversed form. it can be matched against rev_host when available, otherwise we have to invert host and build a rev_host dinamically (bug 843357 should simplify this) > > + "WHEN 1 = ( " \ > > + "SELECT avg(substr(url,1,4) = 'ftp:') = 1 FROM moz_places h " \ > > "SELECT min(...) FROM" seems to work as well and looks clearer, is it slower? > > Also, "= 1" seems redundant since we have "1 = (" in front of the SELECT and > it's clear that we have a boolean expression inside "min()". hm yes, good idea to use min. > + "SELECT avg(substr(url,1,4) = 'ftp:') = 1 FROM moz_places h " \ > > + "WHERE (" _rev_host_predicate ") AND +h.typed = 1 " \ > > "The unary operator + is a no-op. This is not the + operator, this is the "ignore index" operator, +h.typed is a hint to SQLite to not use the typed index here (it should use the rev_host index indeed). It's likely the optimizer may figure that by itself, but it's usually nice to hint when it's clear. > > "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), 0), NEW.typed), " \ > > + "(" HOSTS_PREFIX_PRIORITY_FRAGMENT("rev_host = NEW.rev_host") \ > > How does this expression relate to considering existing pages in the same > group regardless of the presence of the "www." prefix in the host name? > > Is that logic unneeded here, or may the trigger update the "hosts" table > with a result that would be inconsistent with the one given if (for > illustration purposes) we ran the migration step again? good question, I must verify this better, even with a test.

(In reply to Marco Bonardo [:mak] from comment #20) > (In reply to Paolo Amadini [:paolo] from comment #19) > > * Existing pages are found based on a variable named "host", that can match > > the > > * "host" column of the "hosts" table directly, or correspond to another > > * expression in the SQL statement. > > there are no variables in SQLite, it should just be the moz_hosts.host > column here, what made you think it's a variable? In CREATE_PLACES_AFTERINSERT_TRIGGER, I read at some point... "FROM ( " \ "SELECT fixup_url(get_unreversed_host(NEW.rev_host)) AS host " \ ") AS match " \ ...which makes me think that "host" in this case doesn't refer to moz_hosts.host, but to a dynamically created element (variable may be an incorrect term?) named match.host that has only one possible value and exists only in memory, in this case based on NEW.rev_host. > > * @param _rev_host_predicate > > * A SQL expression used to select which records of the moz_places > > table > > * are relevant to the sought host. This is generally obtained by > > * matching the rev_host column against the "host" variable, but > > rev_host > > * can be matched against another expression for efficiency, if the > > host > > * name is already available in reversed form. > > it can be matched against rev_host when available, otherwise we have to > invert host and build a rev_host dinamically (bug 843357 should simplify > this) Ok, feel free to expand the comments as you see fit in the next iteration. > > + "SELECT avg(substr(url,1,4) = 'ftp:') = 1 FROM moz_places h " \ > > > + "WHERE (" _rev_host_predicate ") AND +h.typed = 1 " \ > > > > "The unary operator + is a no-op. > > This is not the + operator, this is the "ignore index" operator, +h.typed is > a hint to SQLite to not use the typed index here (it should use the rev_host > index indeed). It's likely the optimizer may figure that by itself, but it's > usually nice to hint when it's clear. Ok. Too bad SQLite doesn't support the "IGNORE INDEX" syntax, this one is somewhat obscure and hardly documented...

Comment on attachment 719474 [details] [diff] [review] patch v1.2 While waiting for next iteration I'm still reading the code, but I'm clearing the review request on the current version as a reminder.

(In reply to Paolo Amadini [:paolo] from comment #21) > In CREATE_PLACES_AFTERINSERT_TRIGGER, I read at some point... > > "FROM ( " \ > "SELECT fixup_url(get_unreversed_host(NEW.rev_host)) AS host " \ > ") AS match " \ > > ...which makes me think that "host" in this case doesn't refer to > moz_hosts.host, but to a dynamically created element (variable may be an > incorrect term?) named match.host that has only one possible value and > exists only in memory, in this case based on NEW.rev_host. Ah yes, it's an alias. > Ok. Too bad SQLite doesn't support the "IGNORE INDEX" syntax, this one is > somewhat obscure and hardly documented... well, it's because it's not exactly the same, the + has some special behavior, see http://www.sqlite.org/optoverview.html from "erms of the WHERE clause can be manually disqualified" and specifically the note about affinity removal.

Comment on attachment 719474 [details] [diff] [review] patch v1.2 Review of attachment 719474 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marco Bonardo [:mak] from comment #23) > well, it's because it's not exactly the same, the + has some special > behavior, see http://www.sqlite.org/optoverview.html from "erms of the WHERE > clause can be manually disqualified" and specifically the note about > affinity removal. Thanks for the pointer! ::: toolkit/components/places/History.cpp @@ +2256,5 @@ > else if (aFlags & IHistory::REDIRECT_PERMANENT) { > transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT; > } > + else if (recentFlags & nsNavHistory::RECENT_TYPED && > + !(aFlags & IHistory::UNRECOVERABLE_ERROR)) { nit: Eclipse says "Suggested parenthesis around expression 'recentFlags & nsNavHistory::RECENT_TYPED'", and I tend to agree :-) ::: toolkit/components/places/tests/inline/test_trimming.js @@ +7,5 @@ > "mo", > { autoFilled: "mozilla.org/", completed: "https://www.mozilla.org/" }, > function () { > + promiseAddVisits({ uri: NetUtil.newURI("https://www.mozilla.org/test/"), > + transition: TRANSITION_TYPED }); nit: one more space for indentation ::: toolkit/components/places/tests/unit/test_hosts_triggers.js @@ +159,2 @@ > { > function test_removal(aURIToRemove, aURIToKeep, aCallback) { As an optional plus: you might be able to reduce this test code even more by making the "test_removal" function a task itself, and using promiseAddVisits instead of updatePlaces like you did in other test cases. You needn't even change the caller since yielding a generator function spawns a task automatically. This nitpicking ends the detailed review :-) I can mark the next iteration after the expected results for the insert trigger have been worked out.

(In reply to Andrey Belevantsev from comment #14) > 3) if I delete the FTP site from the autocomplete suggestions and I type > 'gcc' in the same tab or new tab, I don't have an FTP site in the list of > suggestions but still I get the FTP site when pressing Enter. > > So for me the only remaining problem is that I'd like to change the > autocomplete behavior to the HTTP version and there is no way to do this, as > point 3) above doesn't work. (I'm not sure whether it is supposed to though). Does the patch fix this issue?

(In reply to Tanvi Vyas [:tanvi] from comment #25) > (In reply to Andrey Belevantsev from comment #14) > > 3) if I delete the FTP site from the autocomplete suggestions and I type > > 'gcc' in the same tab or new tab, I don't have an FTP site in the list of > > suggestions but still I get the FTP site when pressing Enter. > > > > So for me the only remaining problem is that I'd like to change the > > autocomplete behavior to the HTTP version and there is no way to do this, as > > point 3) above doesn't work. (I'm not sure whether it is supposed to though). > > Does the patch fix this issue? The patch makes so if any page exists that is not ftp, we won't suggest ftp. If you remove from autocomplete the ftp page, you remove its history, so if there is no more any ftp page in history it won't be suggested. Though the simple way will be to type http://whatever to enforce going to the http version and once that visit exists, ftp won't be suggested anymore. This behavior will be valid for any other prefix based on discussion in bug 769994. Case 4 reported above may just mean the user had a bookmark to the ftp site, thus it was still suggested, the user can regardless enforce a protocol today, just that it won't stick without this patch.

Sorry, I meant case 3, not case 4.

This should solve previous comments, I added an additional test for the modified trigger. The behavior is different, I moved using the same strategy for all prefixes, so if all typed pages have a prefix, it's suggested. This still satisfies the original bug about having https for a typed https, while allowing users to bypass the behavior just by typing a different protocol once. I think we should try this for now, before investigating even more relaxed solutions, the typed annotation is already a good filter and we only autofill typed entries so the prefix will cope with that better. See also the discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=769994#c112 on

Comment on attachment 760116 [details] [diff] [review] patch v1.3 Review of attachment 760116 [details] [diff] [review]: ----------------------------------------------------------------- I read the conversation in bug 769994 and the changes makes sense in that light, also thanks for the code clean up. ::: toolkit/components/places/nsPlacesTriggers.h @@ +63,2 @@ > */ > +#define HOSTS_PREFIX_PRIORITY_FRAGMENT(_rev_host_predicate) \ Since it now has always the same value, the _rev_host_predicate parameter can now become a constant defined before HOSTS_PREFIX_PRIORITY_FRAGMENT, with its own comment for explanation. I'd also expand the comment here to define: - Which type of SQL is this (an expression, as opposed to a statement) - Which are the external dependencies (in this case, "host", which in our case may refer to "moz_hosts.host" or the dynamically generated "match.host"). I think I don't need to review the patch again after this change.

Comment on attachment 760600 [details] [diff] [review] patch v1.4 [Approval Request Comment] Bug caused by (feature/regressing bug #): 815321, 769994, 862164 User impact if declined: As discussed with Lukas, Gavin, and Marco via email, I'm nominating this bug for uplift since the usability concerns associated with it are heightened with the Mixed Content Blocker in Firefox Aurora. Users will end up at the https version of sites when they intended to visit the http version. Some websites will purposely default to HTTP while they are working on their HTTPS website. Their HTTPS website is under construction and not ready for general users. It may still contain Mixed Content. As an example, when a user visits "https://nytimes.com" they are redirected to "http://www.nytimes.com". This implies that the NY times prefer their users visit the HTTP version of their site for now. However, that redirect is missing when a user visits https://www.nytimes.com (probably an oversite). If the user visits https://www.nytimes.com just once, they will forever be taken to that location instead of http://www.nytimes.com. The HTTPS version has Mixed Content and does not render properly. Without this bug, if a user ends on the HTTPS version once, they will forever be taken to the https version by the awesome bar. Testing completed (on m-c, etc.): Yes, https://tbpl.mozilla.org/?rev=85af7479df1c Risk to taking this patch (and alternatives if risky): We'd ideally like to let it stay in Nightly this week and uplift to Aurora early next week. String or IDL/UUID changes made by this patch: None

Comment on attachment 760600 [details] [diff] [review] patch v1.4 Approving for MCB concerns, let's get this baking on Aurora.

Didn't notice the last part of comment 33. Backed out at Tanvi's request. https://hg.mozilla.org/releases/mozilla-aurora/rev/7c1737dc2232

Adding a flag to the whiteboard "[don't uplift yet]". Per Marco in https://bugzilla.mozilla.org/show_bug.cgi?id=769994#c132, he'd like to see this bake for a few days in Nightly before it hits Aurora because of a database change that can't be undone. I will land to Aurora on Monday.

relanded on Aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/6b40d5226c13

The fix may have caused bug 886064?

(In reply to Aleksej [:Aleksej] from comment #39) > The fix may have caused bug 886064? only if the database was really messed up, and still I have it's hard to figure out what would be going on. I will have to verify at the db level if I can get it.

User Agents: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0 Verified as fixed on Firefox 23 beta 2 (build ID: 20130701144430)

Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 Verified as fixed on Firefox 24 beta 1 (buildID: 20130806170643).