1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Luke Chang [:lchang] (inactive)]

A valid fields (a.k.a non computed fields) with an empty string are supposed to be deleted before saving to the storage.

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage.

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

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8924128 [details]
Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage.

https://reviewboard.mozilla.org/r/195374/#review200868

::: browser/extensions/formautofill/ProfileStorage.jsm:426
(Diff revision 1)
>        // Resume the old field value in the perserve case
>        if (preserveOldProperties && newValue === undefined) {
>          newValue = oldValue;
>        }
>  
> -      if (!newValue) {
> +      if (newValue === undefined || newValue === "") {

It seems no difference after this patch, or is there  any other edge case that we'll delete the recordFound field accidentally?

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8924128 [details]
Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage.

https://reviewboard.mozilla.org/r/195374/#review200910

::: browser/extensions/formautofill/test/unit/test_addressRecords.js:357
(Diff revision 1)
>    do_check_eq(address["street-address"], undefined);
>    do_check_eq(address["postal-code"], "12345");
>    do_check_neq(address.timeLastModified, timeLastModified);
>    do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
>  
> +  // Empty string should be deleted before saving.

nit: Maybe `Empty string should be deleted while updating` because we didn't clean up the empty string at the begining like `add`, but it's really minor anyway.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8924128 [details]
Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage.

https://reviewboard.mozilla.org/r/195374/#review200868

> It seems no difference after this patch, or is there  any other edge case that we'll delete the recordFound field accidentally?

I just wanted to make sure that zero can be saved, although it shouldn't happen in our use case.

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8924128 [details]
Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57072e6ba78b
[Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage. r=steveck

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/57072e6ba78b

Comment 8

[Luke Chang [:lchang] (inactive)]

I just realize this issue is a regression cased by bug 1395122. Since that bug doesn't land on Fx57, this issue doesn't affect Fx57, either.