Closed Bug 1363997 Opened 3 years ago Closed 2 years ago

Add tombstone support to ProfileStorage

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

Sync needs/wants "tombstone" support in the profile storage. In short, deleting an item will leave behind a tombstone with just the GUID and very limited meta-data, and it will be possible to create new tombstones given just the ID and this meta-data.

I'm put a WIP patch up for feedback, but I won't put one up for review until after Luke's refactor lands - which will likely change the patch significantly, but it's still worth getting feedback on the general concepts as early as possible.
Attachment #8866682 - Flags: feedback?(lchang)
Attachment #8866682 - Flags: feedback?(MattN+bmo)
Comment on attachment 8866682 [details]
Bug 1363997 - Add support for tombstones to profileStorage.

https://reviewboard.mozilla.org/r/138292/#review142986

Some questions about the `remove` function.

::: browser/extensions/formautofill/ProfileStorage.jsm:189
(Diff revision 2)
> +      }
> +      recordToSave = {
> +        guid: record.guid,
> +        timeLastModified: record.timeLastModified || Date.now(),
> +        deleted: true,
> +      }

missing `;`.

::: browser/extensions/formautofill/ProfileStorage.jsm:276
(Diff revision 2)
>    /**
>     * Removes the specified record. No error occurs if the record isn't found.
>     *
>     * @param  {string} guid
>     *         Indicates which record to remove.
> +   * @param  {?boolean} [options.keepTombstone = true]

nit: I personally prefer `{boolean}` (without `?`)

::: browser/extensions/formautofill/ProfileStorage.jsm:282
(Diff revision 2)
> +   *         Should we keep a tombstone for this removal?
>     */
> -  remove(guid) {
> +  remove(guid, {keepTombstone = true} = {}) {
>      this.log.debug("remove:", guid);
>  
> -    this._store.data[this._collectionName] =
> +    let index = this._findIndexByGUID(guid, {includeDeleted: !keepTombstone});

Just want to make sure I understand correctly.

1. We set `includeDeleted: true` when `keepTombstone == false`, so we can also remove an existing tombstone from storage. Is there a scenario we want to do this? Would it be clearer to have another API called `removeTombstone(guid)`? 

2. We set `includeDeleted: false` when `keepTombstone == true`, so there will be an exception when we try to remove a record which has been a tombstone. Is it correct?

BTW, I'm just courious if there's a case we want to remove a record but don't want a tombstone?

::: browser/extensions/formautofill/ProfileStorage.jsm:289
(Diff revision 2)
> +      this.log.warn("attempting to remove non-existing entry", guid);
> +      return;
> +    }
> +    if (keepTombstone) {
> +      let existing = this._store.data[this._collectionName][index];
> +      if (existing.deleted) {

Since we set `includeDeleted: false` when `keepTombstone == true`, this condition seems unable to be true.

::: browser/extensions/formautofill/ProfileStorage.jsm:296
(Diff revision 2)
> +      }
> +      this._store.data[this._collectionName][index] = {
> +        guid,
> +        timeLastModified: Date.now(),
> +        deleted: true,
> +      }

missing `;`.

::: browser/extensions/formautofill/ProfileStorage.jsm:332
(Diff revision 2)
>    /**
>     * Returns all records.
>     *
>     * @param   {Object} config
>     *          Specifies how data will be retrieved.
>     * @param   {boolean} config.noComputedFields

Would you mind helping to update this to `{boolean} [config.noComputedFields = false]` to align others? Thanks.

::: browser/extensions/formautofill/ProfileStorage.jsm:334
(Diff revision 2)
>     *
>     * @param   {Object} config
>     *          Specifies how data will be retrieved.
>     * @param   {boolean} config.noComputedFields
>     *          Returns raw record without those computed fields.
> +   * @param   {boolean} config.includeDeleted = false

nit: `{boolean} [config.includeDeleted = false]`

::: browser/extensions/formautofill/ProfileStorage.jsm:335
(Diff revision 2)
>     * @param   {Object} config
>     *          Specifies how data will be retrieved.
>     * @param   {boolean} config.noComputedFields
>     *          Returns raw record without those computed fields.
> +   * @param   {boolean} config.includeDeleted = false
> +   *          Returns raw record without those computed fields.

The description needs an update.

::: browser/extensions/formautofill/test/unit/test_addressRecords_tombstones.js:110
(Diff revision 2)
> +  let profileStorage = new ProfileStorage(path);
> +  await profileStorage.initialize();
> +
> +  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
> +
> +  Assert.throws(() => profileStorage.addresses.update(guid, {}), "No matching profile.");

The message has changed to "No matching record.", the same in test_creditCardRecords_tombstones.js

::: browser/extensions/formautofill/test/unit/test_addressRecords_tombstones.js:113
(Diff revision 2)
> +  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
> +
> +  Assert.throws(() => profileStorage.addresses.update(guid, {}), "No matching profile.");
> +});
> +
> +add_task(async function test_update_tombstone() {

`function test_remove_existing_tombstone()`, the same in test_creditCardRecords_tombstones.js
Attachment #8866682 - Flags: review?(lchang)
(In reply to Luke Chang [:lchang] from comment #3)
> Comment on attachment 8866682 [details]
> 1. We set `includeDeleted: true` when `keepTombstone == false`, so we can
> also remove an existing tombstone from storage. Is there a scenario we want
> to do this? Would it be clearer to have another API called
> `removeTombstone(guid)`? 

Yeah, good point - tests are the only use-case for this and it made the patch more complex. It probably just makes sense for a _wipeAllAndReset test-only function or similar.

> BTW, I'm just courious if there's a case we want to remove a record but
> don't want a tombstone?

As above - it was just to allow a full nuke/reset of items. If we end up needing something in the sync engine or elsewhere that does nuke tombstones, we can address that when we know what it wants.

> Since we set `includeDeleted: false` when `keepTombstone == true`, this
> condition seems unable to be true.

heh, yeah, thanks.

> > +  Assert.throws(() => profileStorage.addresses.update(guid, {}), "No matching profile.");
> 
> The message has changed to "No matching record.", the same in
> test_creditCardRecords_tombstones.js

huh - the tests pass. I'll check it out.

Thanks!
(In reply to Mark Hammond [:markh] from comment #4)
> (In reply to Luke Chang [:lchang] from comment #3)
> > Comment on attachment 8866682 [details]
> > 1. We set `includeDeleted: true` when `keepTombstone == false`, so we can
> > also remove an existing tombstone from storage. Is there a scenario we want
> > to do this? Would it be clearer to have another API called
> > `removeTombstone(guid)`? 
> 
> Yeah, good point - tests are the only use-case for this and it made the
> patch more complex. It probably just makes sense for a _wipeAllAndReset
> test-only function or similar.

I see. "_wipeAllAndReset" sounds better.
Comment on attachment 8866682 [details]
Bug 1363997 - Add support for tombstones to profileStorage.

https://reviewboard.mozilla.org/r/138292/#review142986

> The message has changed to "No matching record.", the same in test_creditCardRecords_tombstones.js

well, TIL of a new foot-gun - that `Assert.throws(f, "message")` is quite different from `Assert.throws(f, /message/)`

> `function test_remove_existing_tombstone()`, the same in test_creditCardRecords_tombstones.js

Fixed, but I decided I wasn't too happy with a simple clone of the "address_tombstone" test to "creditCard_tombstone". The next patch I upload will have test_storage_tombstones.js - a single file that applies each test function to both storage.addresses and storage.creditCards - but happy to hear of a better alternative if you don't like that style.
Comment on attachment 8866682 [details]
Bug 1363997 - Add support for tombstones to profileStorage.

https://reviewboard.mozilla.org/r/138292/#review143746

Looks good. Thanks.

::: browser/extensions/formautofill/ProfileStorage.jsm:321
(Diff revision 3)
>     * @param   {Object} config
>     *          Specifies how data will be retrieved.

nit: we no longer need the info of `config` as it's been destructed.
Attachment #8866682 - Flags: review?(lchang) → review+
Comment on attachment 8866682 [details]
Bug 1363997 - Add support for tombstones to profileStorage.

https://reviewboard.mozilla.org/r/138292/#review144524

Thanks

::: browser/extensions/formautofill/ProfileStorage.jsm:330
(Diff revision 3)
> +   * @param   {boolean} [options.includeDeleted = false]
> +   *          Also return any tombstone records.
>     * @returns {Array.<Object>}
>     *          An array containing clones of all records.
>     */
> -  getAll(config = {}) {
> +  getAll({noComputedFields = false, includeDeleted = false} = {}) {

Do we really need to expose `includeDeleted` on the public API? If it's only for tests then I would rather not. Tests can bypass the public API. I guess "public API" doesn't mean much soon… but for example, I wouldn't want them exposed to a future web extension API probably. I guess it's fine as-is.

::: browser/extensions/formautofill/test/unit/test_storage_tombstones.js:7
(Diff revision 3)
> +Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace");
> +

Did you mean to leave this in? Maybe it should be in the head file?

::: browser/extensions/formautofill/test/unit/test_storage_tombstones.js:7
(Diff revision 3)
> + * Tests tombstones in address/creditcard records.
> + */
> +
> +"use strict";
> +
> +Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace");

Nit: I think setCharPref is kinda deprecated in favour of setStringPref now.
Attachment #8866682 - Flags: review?(MattN+bmo) → review+
FTR, I'm holding off landing this as it seems likely that Kit's reconciliation work might require some changes, so it seems we should get a handle on them to avoid introducing too much churn.
Comment on attachment 8866682 [details]
Bug 1363997 - Add support for tombstones to profileStorage.

https://reviewboard.mozilla.org/r/138292/#review158340
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/cf27236cc52c
Add support for tombstones to profileStorage. r=lchang,MattN
Backed out for failing mochitest test_on_address_submission.html:

https://hg.mozilla.org/integration/autoland/rev/4030f30a29a821dc22c605133eaec893cbd6c656

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cf27236cc52c49e141afca607dbfdf15b9caed90&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110611156&repo=autoland

[task 2017-06-29T01:46:35.823562Z] 01:46:35     INFO - TEST-FAIL | browser/extensions/formautofill/test/mochitest/test_on_address_submission.html | The author of the test has indicated that flaky timeouts are expected.  Reason: Guarantee asynchronous identifyAutofillFields is invoked 
[task 2017-06-29T01:46:35.825330Z] 01:46:35     INFO - Buffered messages logged at 01:41:16
[task 2017-06-29T01:46:35.828222Z] 01:46:35     INFO - TEST-PASS | browser/extensions/formautofill/test/mochitest/test_on_address_submission.html | A valid string reason is expected 
[task 2017-06-29T01:46:35.830070Z] 01:46:35     INFO - TEST-PASS | browser/extensions/formautofill/test/mochitest/test_on_address_submission.html | Reason cannot be empty 
[task 2017-06-29T01:46:35.834240Z] 01:46:35     INFO - TEST-FAIL | browser/extensions/formautofill/test/mochitest/test_on_address_submission.html | The author of the test has indicated that flaky timeouts are expected.  Reason: Guarantee asynchronous identifyAutofillFields is invoked 
[task 2017-06-29T01:46:35.835966Z] 01:46:35     INFO - Buffered messages finished
[task 2017-06-29T01:46:35.840982Z] 01:46:35     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/mochitest/test_on_address_submission.html | Test timed out. 
[task 2017-06-29T01:46:35.842652Z] 01:46:35     INFO -     reportError@SimpleTest/TestRunner.js:121:7
[task 2017-06-29T01:46:35.844342Z] 01:46:35     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2017-06-29T01:46:35.846007Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.847719Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.850375Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.852073Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.853712Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.855407Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.857123Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.858761Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.860584Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.862310Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.864013Z] 01:46:35     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2017-06-29T01:46:35.865634Z] 01:46:35     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
[task 2017-06-29T01:46:35.867232Z] 01:46:35     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
[task 2017-06-29T01:46:35.868925Z] 01:46:35     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
[task 2017-06-29T01:46:35.870525Z] 01:46:35     INFO -     hookupTests@SimpleTest/setup.js:266:5
[task 2017-06-29T01:46:35.872207Z] 01:46:35     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
[task 2017-06-29T01:46:35.873806Z] 01:46:35     INFO -     hookup@SimpleTest/setup.js:246:5
[task 2017-06-29T01:46:35.875590Z] 01:46:35     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp&cleanupCrashes=true:11:1
[task 2017-06-29T01:46:36.258445Z] 01:46:36     INFO - GECKO(1707) | MEMORY STAT | vsize 2079MB | residentFast 210MB | heapAllocated 84MB
Flags: needinfo?(markh)
New try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9082d34c01d84c685c9767e3ed0ab4e072c37f3 seems green, so re-landing.
Flags: needinfo?(markh)
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/97f039ae44d2
Add support for tombstones to profileStorage. r=lchang,MattN
https://hg.mozilla.org/mozilla-central/rev/97f039ae44d2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.