1308515 - Update version of Kinto.js client to 5.0.0

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Ethan Glasser-Camp (:glasserc)]

This version of Kinto.js has a `resetSyncStorage` function in FirefoxStorage, which I will need in my chrome.storage.sync work (bug 1253740).

Comment 1

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1308515: Update version of kinto.js client to 5.0.0,

Review commit: https://reviewboard.mozilla.org/r/84266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84266/

Comment 2

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8798880 [details]
Bug 1308515: Update version of kinto.js client to 5.0.0,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84266/diff/1-2/

Comment 3

[Ethan Glasser-Camp (:glasserc)]

Hmm, seems like this broke on some stuff. I guess I'll have to look more at this..

Comment 4

[Ethan Glasser-Camp (:glasserc)]

I looked at this more today in between meetings. It seems like https://github.com/Kinto/kinto.js/pull/553#pullrequestreview-3556048 is the change that broke blocklist_signatures. It seems like a member variable on `Collection` used to be populated and is now no longer being populated at all. My question is really: do we even need this member variable? Why are we populating it at all? Why are we populating it at the beginning of sync?

:mgoodwin -- do you think it would be OK for me to change blocklist-clients.js to refer to `yield collection.db.getLastModified()` instead of `collection.lastModified`? You already use this in one place so I'm hoping I can use it here too.

:leplatrem -- do you think it would make sense to get rid of `lastModified` as a property of `Collection`? Alternately, would it make sense to load it during/after construction?

Comment 5

[Matthew N. [:MattN]]

Clearing review until the test failure is sorted out

Comment 6

[Mark Goodwin [:mgoodwin]]

(In reply to Ethan Glasser-Camp (:glasserc) from comment #4)
> I looked at this more today in between meetings. It seems like
> https://github.com/Kinto/kinto.js/pull/553#pullrequestreview-3556048 is the
> change that broke blocklist_signatures. It seems like a member variable on
> `Collection` used to be populated and is now no longer being populated at
> all. My question is really: do we even need this member variable? Why are we
> populating it at all? Why are we populating it at the beginning of sync?
> 
> :mgoodwin -- do you think it would be OK for me to change
> blocklist-clients.js to refer to `yield collection.db.getLastModified()`
> instead of `collection.lastModified`? You already use this in one place so
> I'm hoping I can use it here too.

My recollection is that there was some difference between these. I don't recall exactly what. Maybe leplatrem remembers?

Comment 7

[Mathieu Leplatre [:leplatrem]]

>  It seems like a member variable on `Collection` used to be populated and is now no longer being populated at all

The previous implementation in `kinto.js` was not good, it was using this attribute to store some intermediate state during the sync process. 

Now the value should be set at the end of a successful sync process only. 

In our case, here:
https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/services/common/blocklist-clients.js#205
we were relying on a value that was not consistently set, because it was assigned in the middle of failing sync process.

Hence, here we want `collection.db.getLastModified()` which is the timestamp of the last successful sync effectively stored in the local DB.

Comment 8

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

Review commit: https://reviewboard.mozilla.org/r/86134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86134/

Comment 9

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8798880 [details]
Bug 1308515: Update version of kinto.js client to 5.0.0,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84266/diff/2-3/

Comment 10

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a797e7d73f6d

Comment 11

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85032

::: services/common/blocklist-clients.js:206
(Diff revision 1)
>              yield this.validateCollectionSignature(payload, collection, true);
>              // if the signature is good (we haven't thrown), and the remote
>              // last_modified is newer than the local last_modified, replace the
>              // local data
> -            if (payload.last_modified >= collection.lastModified) {
> +            const newLastModified = yield collection.db.getLastModified();
> +            if (payload.last_modified >= newLastModified) {

µnit: instead `newLastModified` I would have put `localLastModified` to match the comment

Comment 12

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86134/diff/1-2/

Comment 13

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8798880 [details]
Bug 1308515: Update version of kinto.js client to 5.0.0,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84266/diff/3-4/

Comment 14

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e4a1639b8a8

Comment 15

[Ethan Glasser-Camp (:glasserc)]

Oops, looks like I failed to post the comment I wanted to post the first time around. From :leplatrem's comments it sounds like I always want to get the last-modified date from the DB, so I've taken the liberty of making that change as a standalone commit and putting it before the update of the kinto.js library.

The first try builds failed for reasons that don't seem related to what I was working on, so I've rebased everything onto the most up-to-date version of the code and repushed to try.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8798880 [details]
Bug 1308515: Update version of kinto.js client to 5.0.0,

https://reviewboard.mozilla.org/r/84266/#review85274

Comment 17

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85448

Comment 18

[Ryan VanderMeulen [:RyanVM]]

MozReview says that the first patch doesn't have sufficient reviews to be autolanded. Please resolve that.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85670

I didn't realize MozReview enforced this.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e4a60eeb1f0
Fix blocklist-clients.js so that it can work with new kinto.js, r=leplatrem,MattN
https://hg.mozilla.org/integration/autoland/rev/a33f036e3496
Update version of kinto.js client to 5.0.0, r=MattN

Comment 21

[Ethan Glasser-Camp (:glasserc)]

Oops! I forgot that :leplatrem doesn't have high enough privileges to r+ a commit by himself. Thanks :MattN for reviewing the other patch.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2e4a60eeb1f0
https://hg.mozilla.org/mozilla-central/rev/a33f036e3496