mozilla
/
gecko-dev
зеркало из https://github.com/mozilla/gecko-dev.git
1
0
Форкнуть 0
Код Задачи Пакеты Проекты Релизы Вики Активность

Bug 1377246 - Move Sync deduping logic into profile storage. r=markh,seanlee 

MozReview-Commit-ID: IdHIg79hboq
This commit is contained in:
Kit Cambridge 2017-07-11 16:34:06 -07:00 коммит произвёл Mark Hammond
Родитель 4953d00b12
Коммит cf8e21261c
5 изменённых файлов: 250 добавлений и 72 удалений
Показать статистику Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

81
browser/extensions/formautofill/FormAutofillSync.jsm
Просмотреть файл

@ -19,43 +19,12 @@ Cu.import("resource://formautofill/FormAutofillUtils.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "profileStorage",
"resource://formautofill/ProfileStorage.jsm");
// XXX - this will end up in ProfileStorage - here for a POC (and at the top
// of the file for eslint)
function findDuplicateGUID(record) {
for (let profile of profileStorage.addresses.getAll()) {
let keys = new Set(Object.keys(record));
for (let key of Object.keys(profile)) {
keys.add(key);
}
let same = true;
for (let key of keys) {
if (!same) {
break;
}
if (profile.hasOwnProperty(key) && record.hasOwnProperty(key)) {
same = profile[key] == record[key];
}
// else something smarter, possibly using version field?
}
if (same) {
return profile.guid;
}
}
return null;
}
// A helper to sanitize address and creditcard entries suitable for logging.
// A helper to sanitize address and creditcard records suitable for logging.
function sanitizeStorageObject(ob) {
// ProfileStorage INTERNAL_FIELDS. These are safe to log as is.
const whitelist = [
"guid",
"version",
"timeCreated",
"timeLastUsed",
"timeLastModified",
"timesUsed",
];
if (!ob) {
return null;
}
const whitelist = ["timeCreated", "timeLastUsed", "timeLastModified"];
let result = {};
for (let key of Object.keys(ob)) {
let origVal = ob[key];
@ -78,14 +47,25 @@ function AutofillRecord(collection, id) {
AutofillRecord.prototype = {
__proto__: CryptoWrapper.prototype,
toEntry() {
return Object.assign({
guid: this.id,
}, this.entry);
},
fromEntry(entry) {
this.id = entry.guid;
this.entry = entry;
// The GUID is already stored in record.id, so we nuke it from the entry
// itself to save a tiny bit of space. The profileStorage clones profiles,
// so nuking in-place is OK.
delete this.entry.guid;
},
cleartextToString() {
// And a helper so logging a *Sync* record auto sanitizes.
let record = this.cleartext;
let result = {entry: {}};
if (record.entry) {
result.entry = sanitizeStorageObject(record.entry);
}
return JSON.stringify(result);
return JSON.stringify({entry: sanitizeStorageObject(record.entry)});
},
};
@ -141,7 +121,7 @@ FormAutofillStore.prototype = {
}
// No matching local record. Try to dedupe a NEW local record.
let localDupeID = /* this.storage. */findDuplicateGUID(remoteRecord.entry);
let localDupeID = this.storage.findDuplicateGUID(remoteRecord.toEntry());
if (localDupeID) {
this._log.trace(`Deduping local record ${localDupeID} to remote`, remoteRecord);
// Change the local GUID to match the incoming record, then apply the
@ -152,28 +132,25 @@ FormAutofillStore.prototype = {
}
// We didn't find a dupe, either, so must be a new record (or possibly
// a non-deleted version of an item we have a tombstone for, which create()
// a non-deleted version of an item we have a tombstone for, which add()
// handles for us.)
this._log.trace("Add record", remoteRecord);
remoteRecord.entry.guid = remoteRecord.id;
this.storage.add(remoteRecord.entry, {sourceSync: true});
let entry = remoteRecord.toEntry();
this.storage.add(entry, {sourceSync: true});
},
async createRecord(id, collection) {
this._log.trace("Create record", id);
let record = new AutofillRecord(collection, id);
record.entry = this.storage.get(id, {
let entry = this.storage.get(id, {
noComputedFields: true,
});
if (!record.entry) {
if (entry) {
record.fromEntry(entry);
} else {
// We should consider getting a more authortative indication it's actually deleted.
this._log.debug(`Failed to get autofill record with id "${id}", assuming deleted`);
record.deleted = true;
} else {
// The GUID is already stored in record.id, so we nuke it from the entry
// itself to save a tiny bit of space. The profileStorage clones profiles,
// so nuking in-place is OK.
delete record.entry.guid;
}
return record;
},

71
browser/extensions/formautofill/ProfileStorage.jsm
Просмотреть файл

@ -696,6 +696,77 @@ class AutofillRecords {
return record._sync;
}
/**
* Finds a local record with matching common fields and a different GUID.
* Sync uses this method to find and update unsynced local records with
* fields that match incoming remote records. This avoids creating
* duplicate profiles with the same information.
*
* @param {Object} record
* The remote record.
* @returns {string|null}
* The GUID of the matching local record, or `null` if no records
* match.
*/
findDuplicateGUID(record) {
if (!record.guid) {
throw new Error("Record missing GUID");
}
if (record.deleted) {
// Tombstones don't carry enough info to de-dupe, and we should have
// handled them separately when applying the record.
throw new Error("Tombstones can't have duplicates");
}
let records = this._store.data[this._collectionName];
for (let profile of records) {
if (profile.deleted) {
continue;
}
if (profile.guid == record.guid) {
throw new Error(`Record ${record.guid} already exists`);
}
if (this._getSyncMetaData(profile)) {
// This record has already been uploaded, so it can't be a dupe of
// another incoming item.
continue;
}
let keys = new Set(Object.keys(record));
for (let key of Object.keys(profile)) {
keys.add(key);
}
// Ignore internal and computed fields when matching records. Internal
// fields are synced, but almost certainly have different values than the
// local record, and we'll update them in `reconcile`. Computed fields
// aren't synced at all.
for (let field of INTERNAL_FIELDS) {
keys.delete(field);
}
for (let field of this.VALID_COMPUTED_FIELDS) {
keys.delete(field);
}
if (!keys.size) {
// This shouldn't ever happen; a valid record will always have fields
// that aren't computed or internal. Sync can't do anything about that,
// so we ignore the dubious local record instead of throwing.
continue;
}
let same = true;
for (let key of keys) {
// For now, we ensure that both (or neither) records have the field
// with matching values. This doesn't account for the version yet
// (bug 1377204).
same = key in profile == key in record && profile[key] == record[key];
if (!same) {
break;
}
}
if (same) {
return profile.guid;
}
}
return null;
}
/**
* Internal helper functions.
*/

26
browser/extensions/formautofill/test/unit/head.js
Просмотреть файл

@ -3,7 +3,7 @@
*/
/* exported getTempFile, loadFormAutofillContent, runHeuristicsTest, sinon,
* initProfileStorage, getSyncChangeCounter
* initProfileStorage, getSyncChangeCounter, objectMatches
*/
"use strict";
@ -13,6 +13,7 @@ var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Cu.import("resource://gre/modules/Services.jsm");
Cu.import("resource://gre/modules/NetUtil.jsm");
Cu.import("resource://gre/modules/ObjectUtils.jsm");
Cu.import("resource://gre/modules/FormLikeFactory.jsm");
Cu.import("resource://testing-common/MockDocument.jsm");
Cu.import("resource://testing-common/TestUtils.jsm");
@ -176,6 +177,29 @@ function getSyncChangeCounter(records, guid) {
return sync.changeCounter;
}
/**
* Performs a partial deep equality check to determine if an object contains
* the given fields.
*
* @param {Object} object
* The object to check. Unlike `ObjectUtils.deepEqual`, properties in
* `object` that are not in `fields` will be ignored.
* @param {Object} fields
* The fields to match.
* @returns {boolean}
* Does `object` contain `fields` with matching values?
*/
function objectMatches(object, fields) {
let actual = {};
for (let key in fields) {
if (!object.hasOwnProperty(key)) {
return false;
}
actual[key] = object[key];
}
return ObjectUtils.deepEqual(actual, fields);
}
add_task(async function head_initialize() {
Services.prefs.setBoolPref("extensions.formautofill.experimental", true);
Services.prefs.setBoolPref("extensions.formautofill.heuristics.enabled", true);

28
browser/extensions/formautofill/test/unit/test_storage_syncfields.js
Просмотреть файл

@ -361,6 +361,34 @@ add_task(async function test_changeGUID() {
ok(!profileStorage.addresses.get(guid_u1), "old guid no longer exists.");
});
add_task(async function test_findDuplicateGUID() {
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
[TEST_ADDRESS_1]);
let [record] = profileStorage.addresses.getAll({rawData: true});
Assert.throws(() => profileStorage.addresses.findDuplicateGUID(record),
/Record \w+ already exists/,
"Should throw if the GUID already exists");
// Add a malformed record, passing `sourceSync` to work around the record
// normalization logic that would prevent this.
let timeLastModified = Date.now();
let timeCreated = timeLastModified - 60 * 1000;
profileStorage.addresses.add({
guid: profileStorage.addresses._generateGUID(),
version: 1,
timeCreated,
timeLastModified,
}, {sourceSync: true});
strictEqual(profileStorage.addresses.findDuplicateGUID({
guid: profileStorage.addresses._generateGUID(),
timeCreated,
timeLastModified,
}), null, "Should ignore internal fields and malformed records");
});
add_task(async function test_reset() {
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
[TEST_ADDRESS_1, TEST_ADDRESS_2]);

116
browser/extensions/formautofill/test/unit/test_sync.js
Просмотреть файл

@ -35,6 +35,11 @@ const TEST_PROFILE_1 = {
email: "timbl@w3.org",
};
const TEST_PROFILE_2 = {
"street-address": "Some Address",
country: "US",
};
function expectLocalProfiles(expected) {
let profiles = profileStorage.addresses.getAll({
rawData: true,
@ -42,15 +47,6 @@ function expectLocalProfiles(expected) {
});
expected.sort((a, b) => a.guid.localeCompare(b.guid));
profiles.sort((a, b) => a.guid.localeCompare(b.guid));
let doCheckObject = (a, b) => {
for (let key of Object.keys(a)) {
if (typeof a[key] == "object") {
doCheckObject(a[key], b[key]);
} else {
equal(a[key], b[key]);
}
}
};
try {
deepEqual(profiles.map(p => p.guid), expected.map(p => p.guid));
for (let i = 0; i < expected.length; i++) {
@ -58,7 +54,7 @@ function expectLocalProfiles(expected) {
let thisGot = profiles[i];
// always check "deleted".
equal(thisExpected.deleted, thisGot.deleted);
doCheckObject(thisExpected, thisGot);
ok(objectMatches(thisGot, thisExpected));
}
} catch (ex) {
do_print("Comparing expected profiles:");
@ -150,6 +146,7 @@ add_task(async function test_outgoing() {
// The tracker should have a score recorded for the 2 additions we had.
equal(engine._tracker.score, SCORE_INCREMENT_XLARGE * 2);
engine.lastSync = 0;
await engine.sync();
Assert.equal(collection.count(), 2);
@ -191,6 +188,7 @@ add_task(async function test_incoming_new() {
// The tracker should start with no score.
equal(engine._tracker.score, 0);
engine.lastSync = 0;
await engine.sync();
expectLocalProfiles([
@ -218,6 +216,7 @@ add_task(async function test_tombstones() {
try {
let existingGUID = profileStorage.addresses.add(TEST_PROFILE_1);
engine.lastSync = 0;
await engine.sync();
Assert.equal(collection.count(), 1);
@ -252,6 +251,7 @@ add_task(async function test_reconcile_both_modified_identical() {
entry: TEST_PROFILE_1,
}), Date.now() / 1000));
engine.lastSync = 0;
await engine.sync();
expectLocalProfiles([{guid}]);
@ -260,7 +260,51 @@ add_task(async function test_reconcile_both_modified_identical() {
}
});
add_task(async function test_dedupe_identical() {
add_task(async function test_incoming_dupes() {
let {server, engine} = await setup();
try {
// Create a profile locally, then sync to upload the new profile to the
// server.
let guid1 = profileStorage.addresses.add(TEST_PROFILE_1);
engine.lastSync = 0;
await engine.sync();
// Create another profile locally, but don't sync it yet.
profileStorage.addresses.add(TEST_PROFILE_2);
// Now create two records on the server with the same contents as our local
// profiles, but different GUIDs.
let guid1_dupe = Utils.makeGUID();
server.insertWBO("foo", "addresses", new ServerWBO(guid1_dupe, encryptPayload({
id: guid1_dupe,
entry: TEST_PROFILE_1,
}), engine.lastSync + 10));
let guid2_dupe = Utils.makeGUID();
server.insertWBO("foo", "addresses", new ServerWBO(guid2_dupe, encryptPayload({
id: guid2_dupe,
entry: TEST_PROFILE_2,
}), engine.lastSync + 10));
// Sync again. We should download `guid1_dupe` and `guid2_dupe`, then
// reconcile changes.
await engine.sync();
expectLocalProfiles([
// We uploaded `guid1` during the first sync. Even though its contents
// are the same as `guid1_dupe`, we keep both.
Object.assign({}, TEST_PROFILE_1, {guid: guid1}),
Object.assign({}, TEST_PROFILE_1, {guid: guid1_dupe}),
// However, we didn't upload `guid2` before downloading `guid2_dupe`, so
// we *should* dedupe `guid2` to `guid2_dupe`.
Object.assign({}, TEST_PROFILE_2, {guid: guid2_dupe}),
]);
} finally {
await cleanup(server);
}
});
add_task(async function test_dedupe_identical_unsynced() {
let {server, engine} = await setup();
try {
// create a record locally.
@ -274,9 +318,11 @@ add_task(async function test_dedupe_identical() {
entry: TEST_PROFILE_1,
}), Date.now() / 1000));
engine.lastSync = 0;
await engine.sync();
// Should no longer have the local guid
// Should have 1 item locally with GUID changed to the remote one.
// There's no tombstone as the original was unsynced.
expectLocalProfiles([
{
guid: remoteGuid,
@ -287,6 +333,35 @@ add_task(async function test_dedupe_identical() {
}
});
add_task(async function test_dedupe_identical_synced() {
let {server, engine} = await setup();
try {
// create a record locally.
let localGuid = profileStorage.addresses.add(TEST_PROFILE_1);
// sync it - it will no longer be a candidate for de-duping.
engine.lastSync = 0;
await engine.sync();
// and an identical record on the server but different GUID.
let remoteGuid = Utils.makeGUID();
server.insertWBO("foo", "addresses", new ServerWBO(remoteGuid, encryptPayload({
id: remoteGuid,
entry: TEST_PROFILE_1,
}), engine.lastSync + 10));
await engine.sync();
// Should have 2 items locally, since the first was synced.
expectLocalProfiles([
{guid: localGuid},
{guid: remoteGuid},
]);
} finally {
await cleanup(server);
}
});
add_task(async function test_dedupe_multiple_candidates() {
let {server, engine} = await setup();
try {
@ -297,29 +372,32 @@ add_task(async function test_dedupe_multiple_candidates() {
// that's OK. We'll write a tombstone for A when we dedupe A to B, and
// overwrite that tombstone when we see A.
let aRecord = {
let localRecord = {
"given-name": "Mark",
"family-name": "Hammond",
"organization": "Mozilla",
"country": "AU",
"tel": "+12345678910",
};
// We don't pass `sourceSync` so that the records are marked as NEW.
let aGuid = profileStorage.addresses.add(aRecord);
let serverRecord = Object.assign({
"version": 1,
}, localRecord);
let bRecord = Cu.cloneInto(aRecord, {});
let bGuid = profileStorage.addresses.add(bRecord);
// We don't pass `sourceSync` so that the records are marked as NEW.
let aGuid = profileStorage.addresses.add(localRecord);
let bGuid = profileStorage.addresses.add(localRecord);
// Insert B before A.
server.insertWBO("foo", "addresses", new ServerWBO(bGuid, encryptPayload({
id: bGuid,
entry: bRecord,
entry: serverRecord,
}), Date.now() / 1000));
server.insertWBO("foo", "addresses", new ServerWBO(aGuid, encryptPayload({
id: aGuid,
entry: aRecord,
entry: serverRecord,
}), Date.now() / 1000));
engine.lastSync = 0;
await engine.sync();
expectLocalProfiles([