1045998 - nsNavHistory::invalidateFrecencies has defective query

Status: RESOLVED INVALID

(Toolkit :: Places, defect)

Description

[JDev]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Background is located here: https://support.mozilla.org/en-US/questions/1012123 

Summary, I found places.sqlite to be storing data all the way back to 2011-2013 on various profiles.  Upon tracing the problem I discovered that if aPlaceIdsQueryString is empty the query runs without setting a frecency value.  When ClearHistory is called it sends an empty value to invalidateFrecencies, and while it's supposed to set a frecency value of -1 in order to properly expire the places.sqlite data, it instead runs "UPDATE moz_places SET frecency = ""WHERE frecency > 0"

Shortened code:
 nsNavHistory::invalidateFrecencies(const nsCString& aPlaceIdsQueryString)
1016 {
1017   // Exclude place: queries by setting their frecency to zero.
1018   nsCString invalidFrecenciesSQLFragment(
1019     "UPDATE moz_places SET frecency = "
1020   );
1021   if (!aPlaceIdsQueryString.IsEmpty())
....
1029   );
1030   if (!aPlaceIdsQueryString.IsEmpty()) {
....
1033     );
1034   }
1035   invalidFrecenciesSQLFragment.AppendLiteral(
1036     "WHERE frecency > 0 "
1037   );



Expected results:

What should happen?  All frecencies >0 should be set to -1 when ClearHistory is called so the next step in the process (expiry) performs its function as expected.

Comment 1

[Marco Bonardo [:mak]]

Thank you, this is very wrong.

Comment 2

[JDev]

(In reply to Marco Bonardo [:mak] from comment #1)
> Thank you, this is very wrong.

heh, this confused me at first.  I thought you were saying I was wrong (which wouldn't be the first time).

I'd have submitted a patch, could be as simple as adding 1 line to handle it, however, I suspect that line was removed previously to fix something else and it would require a larger re-write to fix properly.  Not something I'd want to tackle for my first FF patch.

Comment 3

[Marco Bonardo [:mak]]

ehr, this bug is actually invalid, the first if only refers to NOTIFY_FRECENCY

1018   if (!aPlaceIdsQueryString.IsEmpty())
1019     invalidFrecenciesSQLFragment.AppendLiteral("NOTIFY_FRECENCY(");
1020   invalidFrecenciesSQLFragment.AppendLiteral(
1021       "(CASE "
1022        "WHEN url BETWEEN 'place:' AND 'place;' "
1023        "THEN 0 "
1024        "ELSE -1 "
1025        "END) "
1026   );

Sure, gracing or a newline would have helped readability.

Comment 4

[JDev]

Attachment: Unexplained

Would you mind investigating why it's not working then?  I've attached an image to show not only that negative frecancies are not being deleted, but also that the algorithm is not decaying urls in most cases.  Over 39,000 of the 40,082 have positive frecancies.  Some despite not being visited since the time this bug was filed.

The only time it seems to work is if you manually call "Clear Recent History".

Comment 5

[JDev]

Sorry for the double comment, it's been a while since I filed this and it just clued in: the code in comment 3 is never executed if aPlaceIdsQueryString is empty.  That was the problem.

1018   if (!aPlaceIdsQueryString.IsEmpty())  //this executes when it is NOT empty.

That's why it works when you call "Clear Recent History" because that sets a value for aPlaceIdsQueryString so it's not empty.  When you are invalidating decayed frecencies for cleanup they aren't set to -1, so are never cleaned out of the database.

Reopening for the above reason.

Comment 6

[JDev]

To ensure I was not confused I went back over everything.  I believe I have a bit of a better handle on it.

RemoveAllPages() made me understand that invalidateFrecencies is doing its intended work to simply cause a recalculation to occur by blanking information with the "UPDATE moz_places SET frecency = ""WHERE frecency > 0" query.

places.sqlite data is not expiring and remains in the database indefinitely.  There should be an upper limit to how long the data is stored, especially for HTTPS URLs that might contain private information.  I would suggest that the data expire after 30-90 days but give the user the option to "Keep for XX days" or "Keep Indefinitely" rather than the binary "Remember my browsing and download history" or not.  Firefox is headed in a different direction though so I'm not going to fight for it.