1664252 - Stop breaking redirects chains when expiring visits for long urls

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

We expire visits too long urls to save some database space, unfortunately when those visits are redirect targets, we end up breaking the redirect chain and we cannot recognize anymore the original page as a redirect.

Short term fix may be to just stop expiring visits part of a redirect chain. Long term, we should probably have a better way to track if a visit is a redirect source, rather than having to check if another visit comes from it.

Comment 1

[Marco Bonardo [:mak]]

This affects search mode because in search mode we filter out redirect sources to provide better results, but due to this bug we won't recognize all of them.
And unfortunately the damage to past history is done.

Comment 2

[Marco Bonardo [:mak]]

https://searchfox.org/mozilla-central/rev/eb9d5c97927aea75f0c8e38bbc5b5d288099e687/toolkit/components/places/PlacesExpiration.jsm#161

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1664252 - Stop breaking redirect chains when expiring visits for long urls. r=adw

Comment 4

[Marco Bonardo [:mak]]

While this is not a recent regression, I'd prefer to start doing this sooner than later, thus I'll likely ask for uplift.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbb67f137185
Stop breaking redirect chains when expiring visits for long urls. r=adw

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fbb67f137185

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 9175209 [details]
Bug 1664252 - Stop breaking redirect chains when expiring visits for long urls. r=adw

Beta/Release Uplift Approval Request

• User impact if declined: By expiring some unordered visits, we end up breaking information about redirects, and we're no longer able to distinguish a redirecting url from history. I totally understand it's late in the cycle, and this is not a recent regression, but this is a scary dataloss behavior, because it invalidates assumptions we make to score urls in history for the urlbar. Because we can't fix old and broken history, we can only wait for it to expire, thus, the sooner we fix the logic, the sooner broken history will disappear.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: This is complicate to verify manually, and it's a very specific case that is unlikely to regress once fixed. While it would be testable in automation, since it's really specific the test would be unable to cover other possible causes of it, and this cause is unlikely to happen again. Thus it's not particularly useful. The simplicity of the one-liner patch should do for it.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The trivial patch is limiting a removal, so we're doing less expiration than before, that is pretty much a safe bet.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This expiration invalidates assumptions we make to score urls in history for the urlbar.
• User impact if declined: We end up considering some visits not redirects, and this bumps them up in the urlbar score.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The trivial patch is limiting a removal, so we're doing less expiration than before, that is pretty much a safe bet.
• String or UUID changes made by this patch:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9175209 [details]
Bug 1664252 - Stop breaking redirect chains when expiring visits for long urls. r=adw

We're building the 81 RC today. Let's let this bake for a cycle rather than taking this at the last minute.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9175209 [details]
Bug 1664252 - Stop breaking redirect chains when expiring visits for long urls. r=adw

Approved for 78.4esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/d00c6fc37bc6