892454 - FetchPageInfo returns frecency zero for some visits, causing the visit to not appear in autocomplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

FetchPageInfo before bug 834539 was not reading frecency, now it does, but unfortunately looks like sometimes we get back frecency 0, that means the visit should not be added to autocomplete.
That's wrong, we should get -1 at a maximum, if we get zero we bring it on and on.

Comment 1

[Asaf Romano (gone)]

Actually now that GetPlacesInfo doesn't expose frecency it's ok for FetchPageInfo not to read it at all.

Comment 2

[Marco Bonardo [:mak]]

Yeah, I was mostly trying to figure when we calculate zero frecency to figure if it's an interesting case.
Looks like that happens when we remove a bookmark to a page that has no visits, then we recalculate a 0 frecency, since the page has no visits nor bookmark it makes sense for it to disappear from autocomplete.
The page is then removed asynchronously by expiration.

Another issue of reading frecency is that VisitURI may pass a 0 frecency in some cases (error pages) and that is done so that updateFrecency skips the update later. But now FetchPageInfo overwrites that 0 with the db value.

So the first possibility is to stop reading frecency, we don't need it for now so that probably makes sense, as you said.
The second possibility is for FetchPageInfo to:

  // If the caller passes in a zero frecency, we respect that value and return
  // it.  This happens for error pages we don't want to bump up.
  if (_place.frecency != 0) {
    int32_t frecency = -1;
    rv = stmt->GetInt32(5, &frecency);
    NS_ENSURE_SUCCESS(rv, rv);
    if (frecency != 0) {
      _place.frecency = frecency;
    }
    else if (!IsQueryURI(_place.spec)) {
      // If the database returns a 0 frecency but this page should appear in
      // autocomplete, change it to -1, so later we may update it.
      _place.frecency = -1;
    }
  }

I think for the current use is better to just remove the frecency read, but I should write tests that in future would caught an eventual frecency overwrite or zero.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

In the end I decided for a different approach.
The problem here is that my decision to use a zero frecency to track whether to update frecency was a poor decision, it is error-prone, as we saw, and it reduces usefulness of FetchPageInfo.
Moreover soon or later we may need to read frecency and I'm not willing to remove the partial support we already added here.

So I just barely decided to track the updateFrecency flag apart, so that it can just pass-through fetch.
I also added one mochitest-browser test that is a bare copy of browser_notfound.js, but that checks what happens if a visit already exists, to verify we properly skip updating frecency.
And finally I added a test that verifies if a page has zero frecency and a valid visit is added to it, frecency is properly updated.

Clearly both tests fail without the patch and pass with it.
I'm going to push to Try.

Comment 4

[Marco Bonardo [:mak]]

https://tbpl.mozilla.org/?tree=Try&rev=07f4ce6d9f78

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 774330 [details] [diff] [review]
patch v1.0

one test is still failing, looking into it.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

it was insertion of a new page, since I was not setting anymore frecency to zero, we were not anymore writing zero to the database on a new insert.
The update path doesn't suffer the problem cause we don't yet update frecency at that point.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 774650 [details] [diff] [review]
patch v1.1

Review of attachment 774650 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again for cleaning my mess.

::: toolkit/components/places/History.cpp
@@ +1173,5 @@
>     */
>    nsresult UpdateFrecency(const VisitData& aPlace)
>    {
>      // Don't update frecency if the page should not appear in autocomplete.
> +    if (!aPlace.shouldUpdateFrecency) {

Caller's job. IMO.

::: toolkit/components/places/tests/browser/browser_visited_notfound.js
@@ +4,5 @@
> +
> +const TEST_URI = NetUtil.newURI("http://mochi.test:8888/notFoundPage.html");
> +
> +function test() {
> +  waitForExplicitFinish();

It would be nice to use something more modern here, but if that takes more than 5 minutes, don't do that.

@@ +11,5 @@
> +  registerCleanupFunction(function() {
> +    gBrowser.removeCurrentTab();
> +  });
> +
> +  // First add a visit to the page, this will ensure later we skip updating

"that later"

@@ +12,5 @@
> +    gBrowser.removeCurrentTab();
> +  });
> +
> +  // First add a visit to the page, this will ensure later we skip updating
> +  // frecency on a newly not found page.

"the frecency"; "not-found"

(and maybe s/on/for/ ?)

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addressed comments, though I won't modernize the test at this time cause I want to keep it coherent with the test I cloned and I don't have the will to modernize both at this time.

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4912bbcb5494

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4912bbcb5494

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 776350 [details] [diff] [review]
patch v1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834539
User impact if declined: Some pages don't appear in autocomplete, some other may have a too high rank than expected.
Testing completed (on m-c, etc.): m-c, automated tests
Risk to taking this patch (and alternatives if risky): simple change with tests
String or IDL/UUID changes made by this patch: none

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 776350 [details] [diff] [review]
patch v1.2

low risk patch for a recent regression.Approving on aurora.

Thanks for the tests :)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/602f5a2d621a