Closed Bug 1394704 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(2 files)

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.
Assignee: nobody → selee
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 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)
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 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 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)
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.
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 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 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+
Thanks for your review.
Keywords: checkin-needed
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.