Closed Bug 1286923 Opened 4 years ago Closed 3 years ago

Implement a validator for the sync forms engine

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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: nobody → tchiovoloni
Status: NEW → ASSIGNED
Whiteboard: [data-integrity]
Priority: -- → P2
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
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.
Attachment #8789418 - Flags: review?(markh) → feedback+
(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.
Attachment #8776704 - Attachment is obsolete: true
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+
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
https://hg.mozilla.org/mozilla-central/rev/887a66cc1f77
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.