Ensure we properly handle redirecting and error visits

RESOLVED FIXED in mozilla14

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla14
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Assignee: nobody → mak77
(Assignee)

Comment 1

5 years ago
Just to confirm, the docshell is passing 404 pages to global history through onNewURI
(Assignee)

Comment 2

5 years ago
[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.
(Assignee)

Updated

5 years ago
Summary: Ensure we don't add 404 pages to history → Ensure we don't autocomplete 404 pages
(Assignee)

Updated

5 years ago
Depends on: 738762
(Assignee)

Updated

5 years ago
Blocks: 725714
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 609006 [details] [diff] [review]
patch v1.1
Attachment #608907 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 609007 [details] [diff] [review]
tests v1.1
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 739293
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Ensure we don't autocomplete 404 pages → Ensure we properly handle redirecting and error visits
(Assignee)

Comment 17

5 years ago
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.
Attachment #609006 - Attachment is obsolete: true
Attachment #609007 - Attachment is obsolete: true
Attachment #610274 - Flags: review?(dietrich)
(Assignee)

Comment 18

5 years ago
Created attachment 610275 [details] [diff] [review]
tests v1.2
Attachment #610275 - Flags: review?(dietrich)
(Assignee)

Comment 19

5 years ago
Created attachment 610276 [details] [diff] [review]
kill redirectsMode query option
Attachment #610276 - Flags: review?(dietrich)
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Whiteboard: [needs SR]
Attachment #610274 - Flags: review?(dietrich) → review+
Attachment #610275 - Flags: review?(dietrich) → review+
Attachment #610276 - Flags: review?(dietrich) → review+
(Assignee)

Comment 20

5 years ago
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.
Attachment #610276 - Flags: superreview?(gavin.sharp)
(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?
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Updated

5 years ago
Keywords: addon-compat
Comment on attachment 610276 [details] [diff] [review]
kill redirectsMode query option

thanks for the explanations!
Attachment #610276 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 25

5 years ago
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.
Keywords: dev-doc-needed
Whiteboard: [needs SR]
Target Milestone: --- → mozilla14
Depends on: 743490
https://hg.mozilla.org/mozilla-central/rev/8e494b6b36ef
https://hg.mozilla.org/mozilla-central/rev/52a1ab08417a
https://hg.mozilla.org/mozilla-central/rev/b94616a8d514
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 407760
(Assignee)

Updated

5 years ago
Duplicate of this bug: 772260
Blocks: 772549
(Assignee)

Updated

5 years ago
Depends on: 766799
(Assignee)

Updated

5 years ago
Depends on: 773982
You need to log in before you can comment on or make changes to this bug.