Currently when syncing to a new profile for the first time I get all my form data from the past years added, even though there are many values I have deleted numerous times already. And there are a lot of values! These updates seem to not make it to the server. As I suspected it could be duplicates in the forms database - or maybe not dups, but several entries of e.g. an old email address for different form field names/ids - I installed a plugin in one profile that helps working with the form database. Even spending several hours finding thousand of old values and deleting the entries and forcing a sync afterwards didn't reflect on any other profile. So there are actually two problems I am facing: 1) when I delete a form value in one profile, I expect this change to be synced for all connected profiles (I also asked this question on SUMO https://support.mozilla.org/en-US/questions/1172636) 2) have a better way to work with the forms database to easier remove a specific value everywhere

I don't think there is a good dupe here. We have bug 1346850 which describes *why* this doesn't work. Attachment 8854749 [details] [diff] is a patch that should allow individual deletions (ie, pressing DEL when the popup is open), but there's no identified general solution (ie, specifically the "Clear Recent History" dialog). Ultimately, we probably also want a bug to keep local tombstones so they don't get resurrected by some other device, but that's trickier still. So I think we need 2 bugs - one for the DEL case (which we can fix soon) and one for the more general case. I propose we keep this for the DEL case - it's better than nothing...

Comment on attachment 8911311 [details] Bug 1395427 p1 - Ignore tps logs/reports. https://reviewboard.mozilla.org/r/182790/#review188034 Sounds good to me, I had ignored them in my local config, but we never want these committed, and code in firefox will generate them, so this sounds good.

Comment on attachment 8911314 [details] Bug 1395427 p4 - Add form history items deletions to TPS tests. https://reviewboard.mozilla.org/r/182796/#review188036 This looks fine. I'm a little surprised that we don't have other tests with FormData in them, but well, I won't ask you to add them -- Mostly since I don't know what you'd test that this doesn't cover (collision + deletion is the only thing I can think of, and I'm not sure that actually works, so...). ::: services/sync/tests/tps/test_formdata.js:35 (Diff revision 1) > value: "joe" > } > ]; > > // This is currently pointless - it *looks* like it is trying to check that > -// one of the entries in formdata1 has been removed, but (a) the delete code > +// one of the entries in formdata1 has been removed, but the way the verification I don't think this is pointless anymore, since the 1st isn't true. We do actually want to be able to do a `FormData.verify` on something that doesn't have the deleted entries on it. But maybe I'm misunderstanding. It might not be ideal, but I don't think pointless is correct anymore.

Comment on attachment 8911311 [details] Bug 1395427 p1 - Ignore tps logs/reports. https://reviewboard.mozilla.org/r/182790/#review188228 ::: .gitignore:129 (Diff revision 1) > testing/talos/lib/ > testing/talos/talos/tests/tp5n.zip > testing/talos/talos/tests/tp5n > testing/talos/talos/tests/devtools/damp.manifest.develop > > +# Ignore sync tps logs and reports I got the impression tps writes these logs to the cwd? If that's true, I'm not sure this patch actually makes sense - it only solves the issue for one particular workflow (as IIUC, there's no requirement tps be run from the root of the source tree). TBH, I'd probably rather see an option so the location of the log files can be specified on the command-line and the logs defaulting to $TMP or some such.

Comment on attachment 8911312 [details] Bug 1395427 p2 - Include guid in formhistory-remove notifications. https://reviewboard.mozilla.org/r/182792/#review188230

Comment on attachment 8911314 [details] Bug 1395427 p4 - Add form history items deletions to TPS tests. https://reviewboard.mozilla.org/r/182796/#review188252

Comment on attachment 8911314 [details] Bug 1395427 p4 - Add form history items deletions to TPS tests. https://reviewboard.mozilla.org/r/182796/#review188252 oops - wrong patch :)

Comment on attachment 8911313 [details] Bug 1395427 p3 - Allow form history items to be created with a specific guid. https://reviewboard.mozilla.org/r/182794/#review188248 ::: toolkit/components/satchel/FormHistory.jsm:929 (Diff revision 1) > - "op='add' cannot contain field 'guid'. Either use op='update' " + > - "explicitly or make 'guid' undefined.", > - Cr.NS_ERROR_ILLEGAL_VALUE); > - } else if (change.fieldname && change.value) { > validateOpData(change, "Add"); > } I think we should consider throwing or Cu.reportError() here in an else block - it seems a footgun

> I got the impression tps writes these logs to the cwd? If that's true, I'm not sure this patch actually makes sense - it only solves the issue for one particular workflow (as IIUC, there's no requirement tps be run from the root of the source tree). > TBH, I'd probably rather see an option so the location of the log files can be specified on the command-line and the logs defaulting to $TMP or some such. This gitignore rule also applies to files in sub-directories, so we're covered for the whole tree here. I talked with Thom on IRC about this, changing the default location of the log files might break the TPS automation server, and most of us run tps from inside the tree. Let's go for the quick fix here, and open a follow-up bug to change the default log location.

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cf5ecd16d87e p1 - Ignore tps logs/reports. r=tcsc https://hg.mozilla.org/integration/autoland/rev/553d26ab8ee8 p2 - Include guid in formhistory-remove notifications. r=markh https://hg.mozilla.org/integration/autoland/rev/d7876bfd8bfc p3 - Allow form history items to be created with a specific guid. r=markh https://hg.mozilla.org/integration/autoland/rev/d86f2704b27a p4 - Add form history items deletions to TPS tests. r=markh,tcsc

Backed out for linting failures, e.g. at services/sync/tps/extensions/tps/resource/modules/forms.jsm:219: https://hg.mozilla.org/integration/autoland/rev/4f9b0c2f439dd1bb7a7c8542090b017e2e1ab177 https://hg.mozilla.org/integration/autoland/rev/583edc8f8c18deba37cf0bb1d9416d76ba0839de https://hg.mozilla.org/integration/autoland/rev/366541968f60ab0c967a6bc9892fd2921dcf9959 https://hg.mozilla.org/integration/autoland/rev/fabf7bab5fa89bdfdd78104b9cfed3e65c9482f5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d86f2704b27a11c02a7b46ac5c955ac27ef46f7a&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=133592888&repo=autoland TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/services/sync/tps/extensions/tps/resource/modules/forms.jsm:219:7 | Arrow function expected no return value. (consistent-return) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/satchel/nsFormAutoComplete.js:113:3 | Missing JSDoc parameter type for 'guid'. (valid-jsdoc) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/satchel/nsFormAutoComplete.js:129:11 | Missing trailing comma. (comma-dangle) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/satchel/test/unit/test_notify.js:32:9 | Expected an assignment or function call and instead saw an expression. (no-unused-expressions)

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8aebf21d1a67 p1 - Ignore tps logs/reports. r=tcsc https://hg.mozilla.org/integration/autoland/rev/728cc39c9cf9 p2 - Include guid in formhistory-remove notifications. r=markh https://hg.mozilla.org/integration/autoland/rev/243846e63be2 p3 - Allow form history items to be created with a specific guid. r=markh https://hg.mozilla.org/integration/autoland/rev/aab1bc673fd7 p4 - Add form history items deletions to TPS tests. r=markh,tcsc

https://hg.mozilla.org/mozilla-central/rev/8aebf21d1a67 https://hg.mozilla.org/mozilla-central/rev/728cc39c9cf9 https://hg.mozilla.org/mozilla-central/rev/243846e63be2 https://hg.mozilla.org/mozilla-central/rev/aab1bc673fd7

Thanks everyone for fixing this so quickly. Mark, is this testable in Nightly? If I delete something there, should that change propagate to other profiles (even if they are running in older FF versions?)? (In reply to Mark Hammond [:markh] from comment #1) > So I think we need 2 bugs - one for the DEL case (which we can fix soon) and > one for the more general case. I propose we keep this for the DEL case - > it's better than nothing... Also is this still to be done? I guess the second bug relates to: > 2) have a better way to work with the forms database to easier remove a specific value everywhere

(In reply to Albert Scheiner [:alberts] from comment #25) > Thanks everyone for fixing this so quickly. > > Mark, is this testable in Nightly? If I delete something there, should that > change propagate to other profiles (even if they are running in older FF > versions?)? Yes, I believe that will work. > (In reply to Mark Hammond [:markh] from comment #1) > > So I think we need 2 bugs - one for the DEL case (which we can fix soon) and > > one for the more general case. I propose we keep this for the DEL case - > > it's better than nothing... > > Also is this still to be done? This bug fixes the DEL case, but not the more general "clear recent history" facility. > I guess the second bug relates to: > > > 2) have a better way to work with the forms database to easier remove a specific value everywhere Not really - more like "work out an efficient way to clear lots of history and also tell sync about it" - see bug 1404427 (we really need to clean up some of these bugs - we have dupes everywhere). (Also clearing ni? for Ed about the backout - he's re-landed)