Sync will duplicate history visits.

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

unspecified
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
We truncate the microsecond part of the `date` field before inserting (insertMany requires a Date object, which doesn't allow us to provide microseconds), but we consider it when deduping. This will lead to us roughly doubling the number of history visits D:

Patch incoming.
(Assignee)

Updated

2 months ago
Assignee: nobody → tchiovoloni
Comment hidden (mozreview-request)

Comment 2

2 months ago
mozreview-review
Comment on attachment 8934760 [details]
Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping

https://reviewboard.mozilla.org/r/205648/#review211232

This looks good, but the test seems unclear to me. While I'm sure it's just me not understanding it, it would be great if it could be commented better (and ideally not use time2, unless I *completely* misunderstand it :)

::: services/sync/modules/engines/history.js:298
(Diff revision 1)
>        this._log.error("Error while fetching visits for URL ${record.histUri}", record.histUri);
>      }
>  
>      let i, k;
>      for (i = 0; i < curVisitsAsArray.length; i++) {
> -      curVisits.add(curVisitsAsArray[i].date + "," + curVisitsAsArray[i].type);
> +      // Same logic as used in the loop to generate the key.

This might be clearer as `// Same logic as used in the loop below to generate visitKey.` (It wasn't immediately clear what loop was being referred to given we are in a loop here)

::: services/sync/tests/unit/test_history_engine.js:156
(Diff revision 1)
> +  let {count} = await rawAddVisit(id, "https://www.example.com", time,
> +                                  PlacesUtils.history.TRANSITIONS.TYPED);
> +  equal(count, 1);
> +
> +  let visits = await PlacesSyncUtils.history.fetchVisitsForURL("https://www.example.com");
> +  equal(visits.length, 1);

ISTM we might as well assert that the visitDate here is exactly what we added.

::: services/sync/tests/unit/test_history_engine.js:166
(Diff revision 1)
> +
> +  let wbo = collection.wbo(id);
> +  let data = JSON.parse(JSON.parse(wbo.payload).ciphertext);
> +  equal(data.visits[0].date, time);
> +
> +  let time2 = (Date.now() - oneHourMS / 2) * 1000 + 444;

A comment here would be good to indicate how time2 relates to time - in general, I think this test needs more comments to explain its intent.

(Actually, I'm not sure why time2 is relevant here - IIUC, we should really only need time, as that is the timestamp that ends up duplicated?)

Comment 3

2 months ago
Fwiw, from quite some time we started rounding times in Places. It's possible some code doesn't do that yet, and I'm happy to get bugs filed where we don't.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
mozreview-review-reply
Comment on attachment 8934760 [details]
Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping

https://reviewboard.mozilla.org/r/205648/#review211232

> A comment here would be good to indicate how time2 relates to time - in general, I think this test needs more comments to explain its intent.
> 
> (Actually, I'm not sure why time2 is relevant here - IIUC, we should really only need time, as that is the timestamp that ends up duplicated?)

Without it we fail this check: https://searchfox.org/mozilla-central/source/services/sync/modules/engines.js#1566

(unrelated: the trace statements around that line in reconcile are examples of logs that aren't output without bug 1425544)

Updated

a month ago
Priority: -- → P1

Comment 7

a month ago
mozreview-review
Comment on attachment 8934760 [details]
Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping

https://reviewboard.mozilla.org/r/205648/#review214040

I don't think the test is exercising the new code - it passes even without the changes to the history engine!

::: services/sync/tests/unit/test_history_engine.js:138
(Diff revision 3)
>  
>    deepEqual(engine.toFetch, []);
> +  await PlacesTestUtils.clearHistory();
> +});
> +
> +add_task(async function test_history_visit_roundtrip() {

I don't see where this test is arranging to process a visit with a timestamp that only differs by microseconds and is thus ignored?
Attachment #8934760 - Flags: review?(markh) → review-
Comment hidden (mozreview-request)

Comment 9

a month ago
mozreview-review
Comment on attachment 8934760 [details]
Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping

https://reviewboard.mozilla.org/r/205648/#review214056

Thanks, and sorry for the misunderstanding!
Attachment #8934760 - Flags: review?(markh) → review+

Comment 10

a month ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9723654fda39
Clamp and truncate microseconds from incoming history visit dates when deduping r=markh

Comment 11

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9723654fda39
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.