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

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

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

Updated

2 years ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED

Comment 2

2 years ago
mozreview-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

::: 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

2 years ago
mozreview-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/#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+
Assignee

Comment 4

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

Comment 6

2 years ago
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
Last Resolved: 2 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.