1395427 - Remove single synced form data entries not reflecting in other synced profiles

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Albert Scheiner [:alberts]]

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

Comment 1

[Mark Hammond [:markh] [:mhammond]]

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 2

[Edouard Oger [:eoger]]

Attachment: Bug 1395427 p1 - Ignore tps logs/reports.

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

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1395427 p2 - Include guid in formhistory-remove notifications.

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

Comment 4

[Edouard Oger [:eoger]]

Attachment: Bug 1395427 p3 - Allow form history items to be created with a specific guid.

This allows us to have the same form history GUIDs across devices, which is mandatory
if we want tombstones to actually do something.

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

Comment 5

[Edouard Oger [:eoger]]

Attachment: Bug 1395427 p4 - Add form history items deletions to TPS tests.

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

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

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 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

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 8

[Mark Hammond [:markh] [:mhammond]]

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 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8911312 [details]
Bug 1395427 p2 - Include guid in formhistory-remove notifications.

https://reviewboard.mozilla.org/r/182792/#review188230

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8911314 [details]
Bug 1395427 p4 - Add form history items deletions to TPS tests.

https://reviewboard.mozilla.org/r/182796/#review188252

Comment 11

[Mark Hammond [:markh] [:mhammond]]

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 12

[Mark Hammond [:markh] [:mhammond]]

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

Comment 13

[Edouard Oger [:eoger]]

> 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.

Comment 14

[Edouard Oger [:eoger]]

Comment on attachment 8911311 [details]
Bug 1395427 p1 - Ignore tps logs/reports.

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

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8911312 [details]
Bug 1395427 p2 - Include guid in formhistory-remove notifications.

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

Comment 16

[Edouard Oger [:eoger]]

Comment on attachment 8911313 [details]
Bug 1395427 p3 - Allow form history items to be created with a specific guid.

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

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8911314 [details]
Bug 1395427 p4 - Add form history items deletions to TPS tests.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 20

[Edouard Oger [:eoger]]

Comment on attachment 8911312 [details]
Bug 1395427 p2 - Include guid in formhistory-remove notifications.

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

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8911313 [details]
Bug 1395427 p3 - Allow form history items to be created with a specific guid.

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

Comment 22

[Edouard Oger [:eoger]]

Comment on attachment 8911314 [details]
Bug 1395427 p4 - Add form history items deletions to TPS tests.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Phil Ringnalda (:philor)]

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

Comment 25

[Albert Scheiner [:alberts]]

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

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(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)