1286923 - Implement a validator for the sync forms engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

Similar to the bookmark validator from bug 1265419. This should integrate with telemetry in the same way as the bookmark validator does in bug 1267917.

This is likely to share code with bug 1286918 and bug 1286915, but it's not clear if either of them should block this one right now.

Comment 1

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

Attachment: 0001-Bug-1286923-Implement-sync-validator-for-forms-f-mar.patch

Same comments as bug 1286918, but I'm also not sure about the implementation of normalizeServerItem. This filters out ~200 of what seem like false postives for me, and it seems possible that we should do this in the other validators.

I'm also unsure why after I deleted a form item to test, the getGUID started throwing sometimes... (hence the try/catch). This doesn't appear to be effecting sync (and why it's not isn't really clear to me either).

Comment 2

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

Comment on attachment 8776704 [details] [diff] [review]
0001-Bug-1286923-Implement-sync-validator-for-forms-f-mar.patch

Review of attachment 8776704 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but needs tests.

::: services/sync/modules/engines/forms.js
@@ +269,5 @@
> +    };
> +  }
> +
> +  normalizeServerItem(item) {
> +    // Should this be done in all engines -- using _findDupe directly?

maybe :) I think for most engines other than bookmarks it's probably fine to use _findDupe, but OTOH, I don't mind this too much either.

@@ +279,5 @@
> +    let guid = null;
> +    try {
> +      guid = FormWrapper.getGUID(item.name, item.value);
> +    } catch (e) {
> +      // It isn't clear to me why this sometimes fails...

OTOH though, _findDupe for forms doesn't catch here - it might be worth logging here so we can try and get a handle on why it does actually fail and whether that's a bigger problem we should address for the engine itself (ie, presumably the engine itself would fail too in this case?)

Comment 3

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

Attachment: Bug 1286923 - Implement sync validator for forms and integrate with TPS

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

Comment 4

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

Comment on attachment 8789418 [details]
Bug 1286923 - Implement sync validator for forms and integrate with TPS

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

Comment 5

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

There are no tests since the only code it has different from passwords is the server and client record normalization. If you want I can add tests for though, let me know -- it doesn't seem to be that worth testing to me since they're pretty simple.

I also can't get it to fail in that try/catch anymore, and IIRC the only time I was seeing that was from within about-sync.

That said I have a couple concerns about this patch, namely the fact that we incorrectly report validation errors when private browsing is enabled. At the moment, I think it's a peculiarity of how TPS runs the validator, but it concerns me that it doesn't have the same bug for passwords...

I also am unsure whether or not we should care about all the errors since we don't sync deleted formdata records (bug 568363) -- although it's not clear to me why we don't do this.

Comment 6

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

(In reply to Thom Chiovoloni [:tcsc] from comment #5)
> There are no tests since the only code it has different from passwords is
> the server and client record normalization. If you want I can add tests for
> though, let me know -- it doesn't seem to be that worth testing to me since
> they're pretty simple.

I'm not too worried about that.

> That said I have a couple concerns about this patch, namely the fact that we
> incorrectly report validation errors when private browsing is enabled.

But this does :) I think we need to understand why. Eg, is that also true if the user has 2 windows open, with one being PB?

> I also am unsure whether or not we should care about all the errors since we
> don't sync deleted formdata records (bug 568363) -- although it's not clear
> to me why we don't do this.

This does too. I think at a minimum, the validator should skip reporting these things as a problem.

I notice TPS skips the validation due to PB - does that patch as written cause TPS to report problems in any other tests? I guess it probably doesn't as no history is removed.

Comment 7

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

Comment on attachment 8789418 [details]
Bug 1286923 - Implement sync validator for forms and integrate with TPS

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

Comment 8

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

(In reply to Mark Hammond [:markh] from comment #6)
> (In reply to Thom Chiovoloni [:tcsc] from comment #5)
> > There are no tests since the only code it has different from passwords is
> > the server and client record normalization. If you want I can add tests for
> > though, let me know -- it doesn't seem to be that worth testing to me since
> > they're pretty simple.
> 
> I'm not too worried about that.
> 
> > That said I have a couple concerns about this patch, namely the fact that we
> > incorrectly report validation errors when private browsing is enabled.
> 
> But this does :) I think we need to understand why. Eg, is that also true if
> the user has 2 windows open, with one being PB?

I can't seem to make the validator complain with any combination of private browsing and normal windows (with or without actually showing clientMissing), so I think it might have been a quirk of TPS.

And since we don't want to report the clientMissing entries because of not supporting deletion, its possible I'm just not thinking about it right and these would have been deleted.

Comment 9

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

Comment on attachment 8789418 [details]
Bug 1286923 - Implement sync validator for forms and integrate with TPS

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

Comment 10

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

Comment on attachment 8789418 [details]
Bug 1286923 - Implement sync validator for forms and integrate with TPS

https://reviewboard.mozilla.org/r/77656/#review77512

Awesome

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/887a66cc1f77
Implement sync validator for forms and integrate with TPS r=markh

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/887a66cc1f77