[Form Autofill] Preferences shows the duplicated records after records changed while the address dialog is opening

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Form Manager
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [form autofill:MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
When the address dialog is opening, any "formautofill-storage-changed" makes the all records duplicated.

The double entrant of ManageRecords.loadRecords is the root cause of this issue.
Comment hidden (mozreview-request)
Assignee: nobody → selee
(Assignee)

Comment 2

9 months ago
The observe is triggered by two cases in one submit:
observe: formautofill-storage-changed with data: add
observe: formautofill-storage-changed with data: notifyUsed

I ever consider the loading process is only for "add" or "modify", but the next reentrant of ManageRecords.loadRecords still has the similar issue.
Just help to clarify it. Two or more "formautofill-storage-changed" happening in a very short period will cause "ManageRecords.loadRecords", an async function, to be called before it finishes. Unfortunately some actions in storage trigger multiple "formautofill-storage-changed" events, e.g. "add" and "notifyUsed" are fired together after adding a record.

Comment 4

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review178918

As our offline discussion, ignoring the second `loadRecords` during it's running might cause the rendered result to be out-of-date since we aren't sure whether the second one is related to the first one. Let's discuss it face to face.
Attachment #8902117 - Flags: review?(lchang)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

9 months ago
Hello Luke, Scott,

The patch is modified based on our offline discussion.
If there is any reentrant happens during running loadRecords(), loadRecords() should be called again to make sure the latest changes are applied to the dialog.

This patch is still without a solid mochitest for it yet, and I am working on that.
Please help to review manageDialog code before the test is ready.

Thank you.
Comment hidden (mozreview-request)

Comment 8

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review180264

::: browser/extensions/formautofill/content/manageDialog.js:95
(Diff revision 3)
> +    this._isLoadingRecords = false;
> +    // _loadRecords should be invoked again if there is any multiple entrant
> +    // during running _loadRecords(). This step ensures that the latest request
> +    // still is applied.
> +    while (this._newRequest) {
> +      this._newRequest = false;

I thought `_newRequest`is unable to become `true` again during `_loadRecords` is running here, isn't it? Can we simply call `loadRecords` recursively?
Attachment #8902117 - Flags: review?(lchang)
Comment hidden (mozreview-request)

Comment 10

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review180396

I wonder if it'd be easier if we compute the label first and make renderRecordElements sync. And use timestamp to decide if we want to abort rendering: https://reviewboard.mozilla.org/r/175358/diff/1#index_header
Attachment #8902117 - Flags: review?(scwwu)
(Assignee)

Comment 11

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review180824

::: browser/extensions/formautofill/content/manageDialog.js:95
(Diff revision 3)
> +    this._isLoadingRecords = false;
> +    // _loadRecords should be invoked again if there is any multiple entrant
> +    // during running _loadRecords(). This step ensures that the latest request
> +    // still is applied.
> +    while (this._newRequest) {
> +      this._newRequest = false;

I think `this._isLoadingRecords = false;` is moved after the `while` block can fix this issue.

That means `_newRequest` is `true` if there is any new request coming.
(Assignee)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8903525 [details]
Bug 1394704

https://reviewboard.mozilla.org/r/175358/#review180838

::: browser/extensions/formautofill/content/manageDialog.js:84
(Diff revision 1)
> +    let timeStamp = new Date().getTime();
> +    this._timeStamp = timeStamp;
> +    for (let record of records) {
> +      record.label = await this.getLabel(record);
> +    }
> +    if (this._timeStamp > timeStamp) {

`_timeStamp` is not a reliable way to detect its calling sequence. I would suggest to use a counter to ensure the sequence.
Comment hidden (mozreview-request)

Comment 14

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review180842

Looks good. Thanks for working on this Sean!
Attachment #8902117 - Flags: review?(scwwu) → review+

Comment 15

9 months ago
mozreview-review
Comment on attachment 8902117 [details]
Bug 1394704 - Early return ManageRecords.loadRecords during the loading process.

https://reviewboard.mozilla.org/r/173564/#review180946

::: browser/extensions/formautofill/content/manageDialog.js:80
(Diff revision 4)
> -   * Load records and render them.
> +   * Load records and render them. This function is a wrapper for _loadRecords
> +   * to ensure any reentrant will be handled well.
>     */
>    async loadRecords() {
> +    // This function can be early returned when there is any reentrant happends.
> +    // _newRequest is needed to be marked as `true` for confirming all changes

nit: "_newRequest" needs to be set to ensure all changes will be applied.
Attachment #8902117 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

9 months ago
Thanks for your review.
Keywords: checkin-needed

Comment 18

9 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94f3d2f69206
Early return ManageRecords.loadRecords during the loading process. r=lchang,scottwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94f3d2f69206
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.