Implement a validator for the sync forms engine

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
Rank:
0
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: [data-integrity][measure-integrity][sprint-3])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
Whiteboard: [data-integrity]

Updated

2 years ago
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
Created attachment 8776704 [details] [diff] [review]
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).
Attachment #8776704 - Flags: feedback?(markh)
Whiteboard: [data-integrity] → [data-integrity][measure-integrity]
Whiteboard: [data-integrity][measure-integrity] → [data-integrity][measure-integrity][sprint-3]
Rank: 1
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?)
Attachment #8776704 - Flags: feedback?(markh) → feedback+
Priority: P2 → P1
Rank: 1 → -1
Rank: -1 → 0
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
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.
(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.

Updated

2 years ago
Attachment #8789418 - Flags: review?(markh) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
(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 hidden (mozreview-request)

Updated

2 years ago
Attachment #8776704 - Attachment is obsolete: true

Comment 10

2 years ago
mozreview-review
Comment on attachment 8789418 [details]
Bug 1286923 - Implement sync validator for forms and integrate with TPS

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

Awesome
Attachment #8789418 - Flags: review?(markh) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/887a66cc1f77
Implement sync validator for forms and integrate with TPS r=markh
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/887a66cc1f77
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.