[Form Autofill] Cache input elements as weak reference

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
Right now the cached formfill handler set will store detailed information about the input field, including the element reference. In order to prevent the possible memory leak, we should apply Cu.getWeakReference to create a wear ref and acquire element via get API.
Comment hidden (mozreview-request)
Comment on attachment 8849499 [details]
Bug 1348757 - Apply weakref for the element in cache.

https://reviewboard.mozilla.org/r/122286/#review126546

::: browser/extensions/formautofill/FormAutofillContent.jsm:379
(Diff revision 1)
> -      formHandler.fieldDetails.forEach(detail => this._markAsAutofillField(detail.element));
> +      formHandler.fieldDetails.forEach(detail =>
> +        this._markAsAutofillField(detail.elementRef.get())

I think we should null-check `.get()` here too and not mark if it's gone.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 1)
>        let formatWithElement = {
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
> -        element, // TODO: Apply Cu.getWeakReference and use get API for strong ref.
> +        elementRef: Cu.getWeakReference(element),

To me, `elementRef` means the same thing as `element` and doesn't imply that it's a *weak* ref. I would rather leave it called `element` or be clear that it's a weak reference e.g. `elementWeakRef` or maybe `weakElement` (not sure if the latter is a good name?).

::: browser/extensions/formautofill/FormAutofillHandler.jsm:116
(Diff revision 1)
> -      if (fieldDetail.element === focusedInput ||
> -          fieldDetail.element.value) {
> +      let element = fieldDetail.elementRef.get();
> +      if (element === focusedInput || element.value) {

We need to null-check the return value of .get() in all places. In this case if element !== focusedInput then it will throw evaluating `element.value`
Attachment #8849499 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

7 months ago
mozreview-review-reply
Comment on attachment 8849499 [details]
Bug 1348757 - Apply weakref for the element in cache.

https://reviewboard.mozilla.org/r/122286/#review126546

> I think we should null-check `.get()` here too and not mark if it's gone.

Will do null-check in markAsAutofillField

> To me, `elementRef` means the same thing as `element` and doesn't imply that it's a *weak* ref. I would rather leave it called `element` or be clear that it's a weak reference e.g. `elementWeakRef` or maybe `weakElement` (not sure if the latter is a good name?).

I think `elementWeakRef` might be the better one since it would be more clear.
Comment on attachment 8849499 [details]
Bug 1348757 - Apply weakref for the element in cache.

https://reviewboard.mozilla.org/r/122286/#review127476
Attachment #8849499 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 6

7 months ago
Thanks!
Keywords: checkin-needed

Comment 7

7 months ago
pushed: https://hg.mozilla.org/integration/autoland/rev/495a39e11056e7137b3b82e83e677d4c60994624
Keywords: checkin-needed

Comment 8

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/495a39e11056
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.