Closed Bug 1413479 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

A valid fields (a.k.a non computed fields) with an empty string are supposed to be deleted before saving to the storage.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
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 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.
Attachment #8924128 - Flags: review?(schung) → review+
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.
Pushed by lchang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/57072e6ba78b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: