Looks like in some case we end up doing that, that is really nonsense, the docshell should not pass invalid pages to global history.

Just to confirm, the docshell is passing 404 pages to global history through onNewURI

[22:22] mak bz: smaug: offhand, is it expected that the docshell passes 404 pages to global history? [22:23] smaug I would expect that [22:23] smaug but not sure what actually happens [22:24] mak smaug: what would you expect it to do? I mean, what should history do with 404 pages being unable to recognize them from success pages? [22:25] mak cause, we are storing visits to broken/wrong urls in history... [22:25] mak that doesn't sound sane [22:25] mak so either we block in the docshell, or we make history able to recognize those (thus I'm asking what should I do) [22:25] smaug mak: I do want to know if I've visited some page [22:25] smaug even if that page is 404 [22:26] mak but this way you are autocompleting to mistyped urls [22:29] mak so maybe the answer is to store, but not autocomplete them? this should be feasible with a new TRANSITION_NOTFOUND (though, what error codes should it cover? any 4xx error?) and keeping frecency to zero, at least for common autocomplete. Inline may be a bit more complicated to handle, but maybe just excluding 0 frecency will do.

Sigh, I actually found some more bugs than I wanted while fixing this. Will have to add a test for each change.

This is changing some history behavior: - 404 pages are not autocompleted, but appear in queries, and visits. currently they are autocompleted and appear in queries. - redirect sources (pages redirecting to other pages) are autocompleted, but they don't appear in queries. currently they are autocompleted and appear in queries. - redirect targets (pages not redirecting) are autocompleted and appear in queries. currently they are autocompleted and they are not supposed to appear in queries, but due to a lucky bug they appear. Note that any visit that does not appear in queries is not notified through onVisit, so after the change in case of a redirect only the last page of the chain will be notified, this could be changed in future in case we find it's a problem. This partially breaks redirectsMode query option, though with this change it basically becomes useless (if not to support old entries in the db). I think we could just remove that option and show only targets by default. Thoughts?

Maybe blocking: bug 131193

(In reply to O. Atsushi (Torisugari) from comment #7) > Maybe blocking: bug 131193 Here I'm handling 3XX (as redirects) and 4XX (as client error) reponsens, that bug is more related to 5XX errors. Adding those to the list would not bee hard, but 5XX errors include a bunch of server side issues that are not fault of the user. So I may type in a uri and get a Service Unavailable or Internal Server Error due to an overload, and willing to retry that uri again in minutes. I'm not completely sure they should be completely excluded from autocomplete. Maybe in a follow-up.

(In reply to Marco Bonardo [:mak] from comment #9) In my opinion, autocomplete should remember temporary error pages and should forget permanent error pages. And that applies to temporary redirection and permanent redirection. I agree that most of 4XX (client) are permanent errors and 5XX (server) are temporary errors, but there are some exceptions. "HTTP/1.1 10.5 Server Error 5xx" says: > Except when responding to a HEAD request, the server SHOULD > include an entity containing an explanation of the error > situation, and whether it is a temporary or permanent > condition. That is, some 5XX errors may be permanent error pages. Probably it's too hard for a machine user agent to parse the entity so as to tell whether the error is permanent or not. I guess the OP of bug 131193 would insist that his proxy error is permanent. The problem is the status code was 503, which is a typical temporary error. Then bug 131193 is WONTFIX. However, in that sense, autocomplete should remember NS_ERROR_NET_RESET etc. instead. "Forget all the error pages" is not our strategy any longer. By the way, HTTP status code 429 is going to be a temporary error. http://tools.ietf.org/id/draft-nottingham-http-new-status-04.txt So "4xx or 5xx" seems a rough threshold.

429 is still a client error, the server is just protecting itself, so it is fine to handle it as a client error. I'm distinguishing client side issues to server side issues here. In that it's not really a rough threshold, it's quite good. 5xx errors should be handled in a separate bug, provided we want to handle some of them, I'm not going to handle those here.

Why "server v.s. client" is more important than "temporary v.s. permanent" for autocomplete? I think users don't want to load a mistyped URL again, but he/she may wants to input the URL of 429 pages afterwards.

Show me a practical case where we hit a 429 error, and we may discuss it's usefulness. Server vs client matters cause we don't want to present anything that may be a local fault, while we don't want to block things due to remove faults. I'm not saying this will stay carved in stone, but it's a good starting point, without over-engineering the whole thing.

s/remove/remote/

(In reply to Marco Bonardo [:mak] from comment #13) > Show me a practical case where we hit a 429 error, and we may discuss it's > usefulness. 429 is just a plan now. I think we should block: 501 Not Implemented This is unlikely to fixed in the future. 505 HTTP Version Not Supported This is unlikely to fixed in the future. should not block: 408 Request Timeout Retry will work. 413 Request Entity Too Large Retry will work. (429 Too Many Requests) Retry will work.

My point, if not clear, is that we should not spend time over-engineering solution to uncommon edge-cases, the original bug had never been fixed exactly cause it was trying to reach a useless perfection. Most of these are likely to happen a few times a year, won't make really a difference in the user experience. Btw, this discussion pertains to bug 738762, here we just handle what docshell tells us. So I'm moving the discussion there.

While the underlying bug 738762 may end up changing, this should already be fine to review, just matter of eventually changing the flag name once the dependency is addressed.

Comment on attachment 610276 [details] [diff] [review] kill redirectsMode query option Needs SR on the redirectsMode query option removal. Note that the query system is built so that it ignores unknown keys and proceeds with the known ones. The option is no more needed cause now the most used option is default and the complete result can be gathered through includeHidden=1.

(In reply to Marco Bonardo [:mak] from comment #20) > The option is no more needed cause now the most used option is default and > the complete result can be gathered through includeHidden=1. Just trying to understand the thought process. Is this correct? - basically all in-tree queries that specify redirectsMode pass REDIRECTS_MODE_TARGET (2) - therefore, let's remove the parameter and make REDIRECTS_MODE_TARGET the default behavior (this involves doing some migration cleanup to mark non-REDIRECTS_MODE_TARGET entries "hidden"?) - if you want to include other types of redirects, use includeHidden=1 What happened previously if you didn't specify redirectsMode? I assume you got all redirects? What are the odds that someone was doing that and expecting that behavior? What about the odds of someone making a query with redirectsMode=2&includeHidden=1?

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21) > Just trying to understand the thought process. Is this correct? > - basically all in-tree queries that specify redirectsMode pass > REDIRECTS_MODE_TARGET (2) yes > - therefore, let's remove the parameter and make REDIRECTS_MODE_TARGET the > default behavior (this involves doing some migration cleanup to mark > non-REDIRECTS_MODE_TARGET entries "hidden"?) yes, we do this on idle maintenance really, rather than in migration. This may mean for about a week some view could show some additional redirect source entries. I could move that to migration, but it's not the fastest query, may delay startup by a couple seconds each time we go through the schema update process (either upgrade, or downgrade+upgrade). Imho not worth it for just some additional noise in views for a limited time. Though if migration is needed for the SR, I can do better measures of the perf hit. > - if you want to include other types of redirects, use includeHidden=1 > > What happened previously if you didn't specify redirectsMode? I assume you > got all redirects? yes, both target and sources, but the uses are quite limited, just looking at our codebase, you see everything was excluding sources. I did a search in the addons mxr, and I can only find redirectsMode=2 there. Surely this doesn't cover queries lacking it, but there aren't many use-cases for enumerating redirect sources. > What are the odds that someone was doing that and > expecting that behavior? Will get less entries, but more meaningful ones. I figure out this is a bit subjective, since we don't know what the result was trying to do, but enumerating history is usually done to present it to the user somehow, and redirect sources are not really interesting to him. > What about the odds of someone making a query with > redirectsMode=2&includeHidden=1? I think doesn't make much sense, includeHidden has always been a "yeah I want EVERYTHING" option, excluding then something sounds a bit nonsense.

btw, I did this not just to remove an option that is "mostly used", it's also an expensive condition in our queries, if we mark the sources as hidden from the beginning, the queries are much cheaper.

Comment on attachment 610276 [details] [diff] [review] kill redirectsMode query option thanks for the explanations!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e494b6b36ef https://hg.mozilla.org/integration/mozilla-inbound/rev/52a1ab08417a https://hg.mozilla.org/integration/mozilla-inbound/rev/b94616a8d514 both addon-compat and dev-doc needed are for the behavioral change of nsNavHistoryQuery/nsNavHistoryResult, by default queries will now exclude redirects sources, they can be shown using includeHidden=1. redirectsMode query options has been removed, since most of its uselfulness is now enabled by default.

https://hg.mozilla.org/mozilla-central/rev/8e494b6b36ef https://hg.mozilla.org/mozilla-central/rev/52a1ab08417a https://hg.mozilla.org/mozilla-central/rev/b94616a8d514