Last Comment Bug 737841 - Ensure we properly handle redirecting and error visits
: Ensure we properly handle redirecting and error visits
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla14
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 739293 772260 (view as bug list)
Depends on: 738762 743490 766799 773982
Blocks: 407760 725714 772549
  Show dependency treegraph
 
Reported: 2012-03-21 06:58 PDT by Marco Bonardo [::mak]
Modified: 2012-11-15 04:57 PST (History)
8 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (13.13 KB, patch)
2012-03-23 16:21 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (13.55 KB, patch)
2012-03-24 09:06 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
tests v1.1 (14.92 KB, patch)
2012-03-24 09:07 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (13.63 KB, patch)
2012-03-28 13:30 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
tests v1.2 (15.28 KB, patch)
2012-03-28 13:31 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
kill redirectsMode query option (39.79 KB, patch)
2012-03-28 13:31 PDT, Marco Bonardo [::mak]
dietrich: review+
gavin.sharp: superreview+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2012-03-21 06:58:59 PDT
Looks like in some case we end up doing that, that is really nonsense, the docshell should not pass invalid pages to global history.
Comment 1 Marco Bonardo [::mak] 2012-03-21 14:21:38 PDT
Just to confirm, the docshell is passing 404 pages to global history through onNewURI
Comment 2 Marco Bonardo [::mak] 2012-03-21 14:34:40 PDT
[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.
Comment 3 Marco Bonardo [::mak] 2012-03-23 16:21:32 PDT
Created attachment 608907 [details] [diff] [review]
patch v1.0

Sigh, I actually found some more bugs than I wanted while fixing this.
Will have to add a test for each change.
Comment 4 Marco Bonardo [::mak] 2012-03-24 09:06:51 PDT
Created attachment 609006 [details] [diff] [review]
patch v1.1
Comment 5 Marco Bonardo [::mak] 2012-03-24 09:07:13 PDT
Created attachment 609007 [details] [diff] [review]
tests v1.1
Comment 6 Marco Bonardo [::mak] 2012-03-24 09:13:34 PDT
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?
Comment 7 O. Atsushi (Torisugari) 2012-03-26 08:19:46 PDT
Maybe blocking: bug 131193
Comment 8 Marco Bonardo [::mak] 2012-03-26 11:58:02 PDT
*** Bug 739293 has been marked as a duplicate of this bug. ***
Comment 9 Marco Bonardo [::mak] 2012-03-26 12:05:15 PDT
(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.
Comment 10 O. Atsushi (Torisugari) 2012-03-28 09:54:45 PDT
(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.
Comment 11 Marco Bonardo [::mak] 2012-03-28 10:22:19 PDT
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.
Comment 12 O. Atsushi (Torisugari) 2012-03-28 11:19:08 PDT
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.
Comment 13 Marco Bonardo [::mak] 2012-03-28 11:29:40 PDT
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.
Comment 14 Marco Bonardo [::mak] 2012-03-28 11:29:55 PDT
s/remove/remote/
Comment 15 O. Atsushi (Torisugari) 2012-03-28 12:06:14 PDT
(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.
Comment 16 Marco Bonardo [::mak] 2012-03-28 12:29:23 PDT
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.
Comment 17 Marco Bonardo [::mak] 2012-03-28 13:30:58 PDT
Created attachment 610274 [details] [diff] [review]
patch v1.2

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 18 Marco Bonardo [::mak] 2012-03-28 13:31:19 PDT
Created attachment 610275 [details] [diff] [review]
tests v1.2
Comment 19 Marco Bonardo [::mak] 2012-03-28 13:31:47 PDT
Created attachment 610276 [details] [diff] [review]
kill redirectsMode query option
Comment 20 Marco Bonardo [::mak] 2012-04-05 02:26:26 PDT
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.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-05 11:43:18 PDT
(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?
Comment 22 Marco Bonardo [::mak] 2012-04-06 14:25:30 PDT
(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.
Comment 23 Marco Bonardo [::mak] 2012-04-06 14:26:38 PDT
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 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-06 14:32:47 PDT
Comment on attachment 610276 [details] [diff] [review]
kill redirectsMode query option

thanks for the explanations!
Comment 25 Marco Bonardo [::mak] 2012-04-06 16:22:12 PDT
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.
Comment 27 Marco Bonardo [::mak] 2012-07-09 16:11:14 PDT
*** Bug 772260 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.