412110 - don't bother calculating frecencies for place: urls (they should be 0)

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect, P5)

Description

[(not reading, please use [e-mail] instead)]

don't both calculating frecencies for place: urls

the fix will be to nsNavHistory::InternalAddNewPage()

we currently do:

  // frecency
  PRInt32 frecency = -1;
  if (aCalculateFrecency) {
    rv = CalculateFrecency(aTyped, aVisitCount, 
                           -1 /* no page id, since this page doesn't exist */,
                           url,
                           &frecency);
    NS_ENSURE_SUCCESS(rv, rv);
  }

I think we should do something like:

PRInt32 frecency = -1;
if (IsQueryURL(url))
  frecency = 0;  // place: bookmarks should not show up in autocomplete
else if (aCalculateFrecency) {
  rv = CalculateFrecency(aTyped, aVisitCount, 
                         -1 /* no page id, since this page doesn't exist */,
                         url,
                         &frecency);
  NS_ENSURE_SUCCESS(rv, rv);
}

Note that if aCalculateFrecency is false, we use -1 for frecency to show that it is invalid, and need to be calculated (on idle) but it will still show up in autocomplete.

for place: urls, frececny should always be 0, to prevent them from showing up in autocomplete.

note, even when we fix this, we still need the code in FixInvalidFrecenciesForExcludedPlaces() to fix place: queries, as we might have set a place: query to -1 on clear all private data or from migration from fx 3b2.

another note:  it might be possible that something is calling CalculateFrecencyInternal() directly with a place: url.

Here's a stack showing how this can happen:

 	places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=-1, int aIsBookmarked=0, int * aFrecency=0x0012c468)  Line 6123	C++
 	places.dll!nsNavHistory::CalculateFrecency(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=-1, nsCAutoString & aURL={...}, int * aFrecency=0x0012c468)  Line 6196 + 0x20 bytes	C++
>	places.dll!nsNavHistory::InternalAddNewPage(nsIURI * aURI=0x00fb5c10, const nsAString_internal & aTitle={...}, int aHidden=1, int aTyped=0, int aVisitCount=0, int aCalculateFrecency=1, __int64 * aPageID=0x0012c740)  Line 1481 + 0x25 bytes	C++
 	places.dll!nsNavHistory::GetUrlIdFor(nsIURI * aURI=0x00fb5c10, __int64 * aEntryID=0x0012c740, int aAutoCreate=1)  Line 1408 + 0x1c bytes	C++
 	places.dll!nsNavBookmarks::InsertBookmark(__int64 aFolder=8, nsIURI * aItem=0x00fb5c10, int aIndex=-1, const nsACString_internal & aTitle={...}, __int64 * aNewBookmarkId=0x0012c988)  Line 874 + 0x19 bytes	C++

Comment 1

[Seth Spitzer]

here's an example of where we call CalculateFrecenyInternal() directly:

 	places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=51, int aIsBookmarked=0, int * aFrecency=0x0012c55c)  Line 6121	C++
 	places.dll!nsNavHistory::UpdateFrecency(__int64 aPlaceId=51, int aIsBookmarked=0)  Line 5999 + 0x23 bytes	C++
>	places.dll!nsNavBookmarks::InsertBookmark(__int64 aFolder=60, nsIURI * aItem=0x069d2488, int aIndex=-1, const nsACString_internal & aTitle={...}, __int64 * aNewBookmarkId=0x0012c988)  Line 947 + 0x1b bytes	C++

That happens when creating a bookmark.

Comment 2

[Seth Spitzer]

this will also happen when you change a place: bookmark

 	places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=53, int aIsBookmarked=1, int * aFrecency=0x00129478)  Line 6121	C++
 	places.dll!nsNavHistory::UpdateFrecency(__int64 aPlaceId=53, int aIsBookmarked=1)  Line 5999 + 0x23 bytes	C++
>	places.dll!nsNavBookmarks::ChangeBookmarkURI(__int64 aBookmarkId=10, nsIURI * aNewURI=0x06c8a1b0)  Line 2197 + 0x19 bytes	C++

Comment 3

[Marco Bonardo [:mak]]

the pointed out code is quite old, I think nowadays (I quickly went through the code and looks like I'm recalling correctly) we are doing the right thing (we set frecency to 0 and never update 0 frecencies).