Closed Bug 1448064 Opened 6 years ago Closed 6 years ago

Replace services/sync usages of updatePlaces with PlacesUtils.history.insert(Many) or PlacesTestUtils.addVisits

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: tcsc)

References

Details

(Whiteboard: [fxsearch][lang=js])

Attachments

(1 file, 1 obsolete file)

There's two remaining instances where we use .updatePlaces in services/sync:

https://searchfox.org/mozilla-central/search?q=.updatePlaces(&case=false&regexp=false&path=services

One of these is a test file and should use PlacesTestUtils.addVisits, the other should use the PlacesUtils.history.insertMany.

Since both of these are wrapped in new Promise wrappers in async functions, the wrappers can be simply removed and the async functions handled directly.

See dependencies of bug 1448041 for examples of replacements.
test_history_engine.js actually can't really be changed to use insertMany. It uses updatePlaces because (and not insertMany or similar) updatePlaces won't truncate microseconds to milliseconds, but insertMany (and AFAICT addVisits is the same here).

It's testing we don't add duplicate visits when one has microsecond precision and the other millisecond precision, since navigation adds visits with microsecond timestamp, but insertMany (and by extension, sync) will add items with milliseconds. Prior to fixing Bug 1423395, sync would duplicate almost every visit done locally!

I'd rather not remove the test (since avoiding the duplication is at least somewhat subtle), is there a version of the API that can insert microseconds? Or should I just insert the item, then update it directly using SQL?
Flags: needinfo?(standard8)
Err, upon rereading, that first paragraph got away from me a bit, it should say:

test_history_engine.js actually can't really be changed to use insertMany. It uses updatePlaces (and not insertMany or similar) because updatePlaces won't truncate microseconds to milliseconds, but insertMany will (and AFAICT addVisits is the same here).
I'm going to pass you over to Marco, as I'm not quite sure if we're keeping the API or not. I would suspect doing SQL is probably the thing to do, but I'm not totally sure.
Flags: needinfo?(standard8) → needinfo?(mak77)
Yes, we are keeping updatePlaces, the reason to move to insert and insertMany is that I'd like to make updatePlaces an internal implementation detail, rather than an API, something we could change at any time for perf reasons.

(In reply to Thom Chiovoloni [:tcsc] from comment #1)
> test_history_engine.js actually can't really be changed to use insertMany.
> It uses updatePlaces because (and not insertMany or similar) updatePlaces
> won't truncate microseconds to milliseconds, but insertMany (and AFAICT
> addVisits is the same here).

Places is supposed to truncates all timestamps to milliseconds internally, it's likely somewhere we don't yet (history is one of those places if I read it correctly). That's a bug.
I already pointed this out at https://bugzilla.mozilla.org/show_bug.cgi?id=1423395#c3
The long term scope was to move to milliseconds (or even tens of milliseconds) in the db itself when the rounding was complete everywhere. Though, it may be complicate to support downgrades, so I can't tell if it will ever happen.
Anyway most of the consumers are js and don't support microseconds, thus keeping microseconds precision doesn't make much sense.

To sum up, the problem here seems the fact we don't round visits time properly, if Sync would start rounding them, it surely would start creating duplicates. The only way out I see is:
1. fix Places to always round to ms
2. migrate not rounded visits newer than the sync time threshold (if there's one)
3. stop using updatePlaces in Sync
Flags: needinfo?(mak77)
Mentor: standard8
Flags: needinfo?(tchiovoloni)
Mak: Sync already properly handles the rounding that insertMany does (that was bug 1423395), and doesn't use updatePlaces outside of TPS (which can be fixed without issues) and test_history_engine.js (which is the file in question. 

This test file just ensures that we don't duplicate when there is a visit with a microsecond timestamp in the Places DB. To do that, we need to insert one, which we do using updatePlaces.

The solutions I see of this are to either 

1. Keep using updatePlaces in the test (which only makes sense if updatePlaces isn't going to be removed)

2. Use SQL directly to insert a visit with a microsecond timestamp (or to update an existing visit's timestamp so that it's not aligned to a millisecond -- either way would work, obviously).

3. have places round all visits, and migrate existing visits to rounded (as you suggest). This seems like it wouldn't be a quick fix, so I'm going to assume it's not how we should address this test.

I'm going to assume that #2 is what we want, since it solves the issue of sync using updatePlaces, but if you'd rather us use updatePlaces than update the DB directly (only in this test!), that's certainly an easier fix.
Flags: needinfo?(tchiovoloni)
Assignee: nobody → tchiovoloni
Comment on attachment 8962907 [details]
Bug 1448064 - (part 2) Avoid using updatePlaces in sync's test_history_engine.js

Let me know if this is acceptable. If you'd rather us keep using updatePlaces in this test over accessing the DB using SQL, that's fine too.
Attachment #8962907 - Flags: feedback?(mak77)
(In reply to Thom Chiovoloni [:tcsc] from comment #5)
> This test file just ensures that we don't duplicate when there is a visit
> with a microsecond timestamp in the Places DB.

That's why I think we should remove all of them from the DB, then there's no such a case.

> 1. Keep using updatePlaces in the test (which only makes sense if
> updatePlaces isn't going to be removed)
> 
> 2. Use SQL directly to insert a visit with a microsecond timestamp (or to
> update an existing visit's timestamp so that it's not aligned to a
> millisecond -- either way would work, obviously).

these are pretty much the same, and since my general rule is "anything writing to places.sqlite must live in toolkit/components/places", it's better to keep using updatePlaces until we are ready.

> 3. have places round all visits, and migrate existing visits to rounded (as
> you suggest). This seems like it wouldn't be a quick fix, so I'm going to
> assume it's not how we should address this test.

Yes, that's what we should do. let's not fix this test.
Comment on attachment 8962906 [details]
Bug 1448064 - Use PlacesUtils.history.insert instead of updatePlaces in TPS

https://reviewboard.mozilla.org/r/231708/#review237368

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:56
(Diff revision 1)
>     */
> -  async Add(item, usSinceEpoch) {
> +  async Add(item, msSinceEpoch) {
>      Logger.AssertTrue("visits" in item && "uri" in item,
>        "History entry in test file must have both 'visits' " +
>        "and 'uri' properties");
> -    let uri = Services.io.newURI(item.uri);
> +    let url = Services.io.newURI(item.uri);

Nit: I think we can drop `Services.io.newURI`.

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:88
(Diff revision 1)
>        "History entry in test file must have both 'visits' " +
>        "and 'uri' properties");
>      let curvisits = await PlacesSyncUtils.history.fetchVisitsForURL(item.uri);
>      for (let visit of curvisits) {
>        for (let itemvisit of item.visits) {
> -        let expectedDate = itemvisit.date * 60 * 60 * 1000 * 1000
> +        let expectedDate = itemvisit.date * 60 * 60 * 1000 * 1000 + msSinceEpoch * 1000;

Maybe a comment that `itemvisit.date` is in microseconds here would be good? OTOH, I guess it's implied by `msSinceEpoch * 1000`...

::: services/sync/tps/extensions/tps/resource/tps.jsm:380
(Diff revision 1)
>    async HandleForms(data, action) {
>      this.shouldValidateForms = true;
>      for (let datum of data) {
>        Logger.logInfo("executing action " + action.toUpperCase() +
>                       " on form entry " + JSON.stringify(datum));
> -      let formdata = new FormData(datum, this._usSinceEpoch);
> +      let formdata = new FormData(datum, this._msSinceEpoch);

Does https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/services/sync/tps/extensions/tps/resource/modules/forms.jsm#121 need to be changed, too?
Attachment #8962906 - Flags: review?(kit) → review+
Comment on attachment 8962907 [details]
Bug 1448064 - (part 2) Avoid using updatePlaces in sync's test_history_engine.js

https://reviewboard.mozilla.org/r/231710/#review237372

This LGTM, but I'll leave it to Mak for the final stamp of approval.

::: services/sync/tests/unit/test_history_engine.js:15
(Diff revision 1)
> -    let results = [];
> -    let handler = {
> -      handleResult(result) {
> -        results.push(result);
> -      },
> -      handleError(resultCode, placeInfo) {
> +async function rawAddVisit(guid, uri, visitPRTime, transitionType) {
> +  let roundedDate = PlacesUtils.toDate(visitPRTime);
> +  // Insert the visit with a rounded date.
> +  await PlacesUtils.history.insert({
> +    guid,
> +    url: CommonUtils.makeURI(uri),

Nit: Just `uri`.
Attachment #8962907 - Flags: review?(kit)
Attachment #8962907 - Flags: review?(mak77)
Comment on attachment 8962907 [details]
Bug 1448064 - (part 2) Avoid using updatePlaces in sync's test_history_engine.js

D'oh. Comment 9.
Attachment #8962907 - Flags: review?(mak77)
Attachment #8962907 - Flags: feedback?(mak77)
Priority: -- → P2
Attachment #8962907 - Attachment is obsolete: true
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/951167bd0647
Use PlacesUtils.history.insert instead of updatePlaces in TPS r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/951167bd0647
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.