Convert the history engine to use `PlacesUtils.history.insertMany`

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: lina, Assigned: lyret, Mentored)

Tracking

(Depends on: 1 bug, {good-first-bug})

unspecified
Firefox 57
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
The history engine has a hand-rolled wrapper around `mozIAsyncHistory.updatePlaces`: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/services/sync/modules/engines/history.js#293-321

Let's replace this wrapper with the nicer, promise-based `PlacesUtils.history.insertMany`: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/toolkit/components/places/History.jsm#212-255

This is a great first bug, but we should wait for bug 1210296 to land before working on this.
Priority: -- → P3
Description says good-first-bug, but is it good-second-bug too? Let's find out.

Mind if I pick this up? What are the relevant tests for this change?
Flags: needinfo?(markh)
(Assignee)

Comment 2

a year ago
I started to work yesterday in this bug and i found so far two test:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

with xpcshell, you can test: PlaceUtils.history.insertMany

and the last one:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/TPS_Tests

And here you can test test_history.js but i found troubles with tps test. i don't know if i'm doing something wrong or documentation is outdated but my test always fail. (example test too!)
(In reply to Luciano I from comment #2)
> I started to work yesterday in this bug and i found so far two test:
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests
> 
> with xpcshell, you can test: PlaceUtils.history.insertMany

Note that this bug isn't about testing .insertMany, it's about shifting the core of the sync code to use that. It would be fine if tests (including TPS) didn't move to using this - and in some ways may be preferred, as keeping the tests the same helps build confidence that the semantics of the history engine haven't changed) - getting the tests moved over eventually might make sense, but I think it's fine to ignore for now.

(In reply to Nicolas Ouellet-Payeur from comment #1)
> Description says good-first-bug, but is it good-second-bug too? Let's find
> out.
> 
> Mind if I pick this up? What are the relevant tests for this change?

I think we should give Luciano a chance to upload a patch - if there's been no movement for a while I think it's OK to take. Another bug that might interest you and might be suitable is bug 1295510 (and maybe bug 1135943, bug 1346072, bug 1353217, bug 1371746 or bug 1375223 :)

Thanks for you interest!
Flags: needinfo?(markh)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8891590 [details]
Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'.

https://reviewboard.mozilla.org/r/162702/#review168116

Thanks very much for the patch, Luciano, it's a good first cut! r- because this won't work for multiple visits yet, and needs some changes to the `VisitInfo` property names.

Please fix up the issues, make sure the tests are green (`./mach xpcshell-test services/sync/test_history` will run just the history tests; if those pass, you can run `./mach xpcshell-test services/sync` to make sure the others pass, too), and that `./mach eslint services/sync` doesn't report any issues.

Then, amend the commit, push a new version to MozReview, and we can get it landed. Thanks again for working on this.

::: services/sync/modules/engines/history.js:272
(Diff revision 1)
>  
>        try {
>          if (record.deleted) {
>            let promise = this.remove(record);
>            promise = promise.catch(ex => failed.push(record.id));
>            blockers.push(promise);

While you're here, let's remove the `blockers` array entirely. Our storage layer serializes all SQLite statements, so accumulating the promises into an array and then `await`-ing them at the end doesn't improve throughput.

Let's simplify this block to just `await this.remove(record)`. The outer `catch` already adds the ID to `failed` if this throws, so we can also drop the `promise.catch` line.

::: services/sync/modules/engines/history.js:296
(Diff revision 1)
>      records.length = k; // truncate array
>  
> +    // Need it for PageInfo Validation
> +    for(i=0; i<records.length; i++) {
> +      let record = records[i];
> +      let record_visit = record.visits[0];

Unfortunately, this only handles the first visit. `insertMany` will still throw if we have more than one visit for a URL.

However, I think `_recordToPlaceInfo` is a better place for this logic, anyway, since we already rewrite the record's properties there. See my comments below.

::: services/sync/modules/engines/history.js:299
(Diff revision 1)
> +    for(i=0; i<records.length; i++) {
> +      let record = records[i];
> +      let record_visit = record.visits[0];
> +
> +      record.url = record.histUri;
> +      record_visit.date = PlacesUtils.toDate(record_visit.date);

Nice! These two lines are correct, but let's move them to `_recordToPlaceInfo`.

::: services/sync/modules/engines/history.js:300
(Diff revision 1)
> +      let record = records[i];
> +      let record_visit = record.visits[0];
> +
> +      record.url = record.histUri;
> +      record_visit.date = PlacesUtils.toDate(record_visit.date);
> +      record_visit.visitDate = PlacesUtils.toDate(record_visit.visitDate);

I don't think this is necessary, though...`visitDate` is a legacy `mozIVisitInfo` property, but `insertMany` takes plain JavaScript `VisitInfo` objects.

::: services/sync/modules/engines/history.js:304
(Diff revision 1)
> +      record_visit.date = PlacesUtils.toDate(record_visit.date);
> +      record_visit.visitDate = PlacesUtils.toDate(record_visit.visitDate);
> +    }
> +
>      if (records.length) {
> -      blockers.push(new Promise(resolve => {
> +      blockers.push(

As above, let's drop `blockers` and just do `await PlacesUtils.history.insertMany(records)`.

::: services/sync/modules/engines/history.js:325
(Diff revision 1)
>     * returns true if the record is to be applied, false otherwise
>     * (no visits to add, etc.),
>     */
>    _recordToPlaceInfo: function _recordToPlaceInfo(record) {
>      // Sort out invalid URIs and ones Places just simply doesn't want.
>      record.uri = Utils.makeURI(record.histUri);

`VisitInfo` calls this `url`, not `uri`. Let's change this line to read `record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri)`, and remove the `if (!record.uri) { ... }` block below.

`normalizeToURLOrGUID` will throw if `histUri` isn't valid, and avoid an unnecessary `nsIURI` conversion.

::: services/sync/modules/engines/history.js:380
(Diff revision 1)
>          // Visit is a dupe, don't increment 'k' so the element will be
>          // overwritten.
>          continue;
>        }
>  
>        visit.visitDate = visit.date;

Let's replace this line with the one you added to `applyIncomingBatch`: `visit.date = PlacesUtils.toDate(visit.date)`.

::: services/sync/modules/engines/history.js:381
(Diff revision 1)
>          // overwritten.
>          continue;
>        }
>  
>        visit.visitDate = visit.date;
>        visit.transitionType = visit.type;

According to the list of visit properties (https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.jsm#46-57), I think this should be `transition`, not `transitionType`.
Attachment #8891590 - Flags: review?(kit) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8891590 [details]
Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'.

https://reviewboard.mozilla.org/r/162702/#review168238

Thanks! Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f1aa1211e2 Fix up the two remaining issues, and then we can land this. \o/

::: services/sync/modules/engines/history.js:307
(Diff revision 2)
>     * (no visits to add, etc.),
>     */
>    _recordToPlaceInfo: function _recordToPlaceInfo(record) {
>      // Sort out invalid URIs and ones Places just simply doesn't want.
> +    record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri);
>      record.uri = Utils.makeURI(record.histUri);

Drop the `record.uri` line. It's not needed now that you're setting `record.url` above, and `insertMany` will ignore it.

::: services/sync/modules/engines/history.js:359
(Diff revision 2)
>          // overwritten.
>          continue;
>        }
>  
> +      visit.date = PlacesUtils.toDate(visit.date);
>        visit.visitDate = visit.date;

Drop `visitDate`, too.
Attachment #8891590 - Flags: review?(kit)
(Assignee)

Comment 8

a year ago
mozreview-review
Comment on attachment 8891590 [details]
Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'.

https://reviewboard.mozilla.org/r/162702/#review168376

::: services/sync/modules/engines/history.js:307
(Diff revision 2)
>     * (no visits to add, etc.),
>     */
>    _recordToPlaceInfo: function _recordToPlaceInfo(record) {
>      // Sort out invalid URIs and ones Places just simply doesn't want.
> +    record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri);
>      record.uri = Utils.makeURI(record.histUri);

if i drop that line, this test no pass:
http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_history_store.js#64
(Reporter)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8891590 [details]
Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'.

https://reviewboard.mozilla.org/r/162702/#review168376

> if i drop that line, this test no pass:
> http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_history_store.js#64

Right, you'll want to remove the `canAddURI` call in that function, and other references to `record.uri`.

But...I see that `insertMany` will throw if none of the incoming history records can be stored, which will cause the "javascript:" URL test to fail. I filed bug 1385989 to see if we can change `insertMany` to behave differently here.

For now, let's keep `record.uri` and `canAddURI`, and remove them in a follow-up. Thanks for double-checking!
Comment hidden (mozreview-request)
(Reporter)

Comment 11

a year ago
mozreview-review
Comment on attachment 8891590 [details]
Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'.

https://reviewboard.mozilla.org/r/162702/#review168482

Thanks!
Attachment #8891590 - Flags: review?(kit) → review+

Comment 12

a year ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26831e829c5a
Converts the history engine to use 'PlacesUtils.history.insertMany'. r=kitcambridge

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26831e829c5a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1388149
Assignee: nobody → lyret
Depends on: 1388024
Forgot the part where I was supposed to comment about what that was, but that was a backout from beta at markh's request.

It hasn't yet been backed out on the trunk.
status-firefox56: fixed → ---
status-firefox57: --- → fixed
Target Milestone: Firefox 56 → Firefox 57
And backed out of 57 in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4b07fd0391
Status: RESOLVED → REOPENED
status-firefox57: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---

Comment 17

a year ago
backoutbugherder
also backedout from m-c
https://hg.mozilla.org/mozilla-central/rev/9b4b07fd0391
It appears the symptoms in bug 1388024 aren't actually due to this bug. Kit, do you think we should just reland this?
Flags: needinfo?(kit)
(Reporter)

Comment 19

a year ago
Sure, let's reland this in 57. Good catch on bug 1390178!
Flags: needinfo?(kit)
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 20

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5322ba58976c
Converts the history engine to use 'PlacesUtils.history.insertMany'. r=kitcambridge
Keywords: checkin-needed
ni myself to look at telemetry for the history engine once this has baked for a while.
Flags: needinfo?(markh)

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5322ba58976c
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1405566
(In reply to Mark Hammond [:markh] from comment #21)
> ni myself to look at telemetry for the history engine once this has baked
> for a while.

I'm glad I did - bug 1405566.
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.