1348757 - [Form Autofill] Cache input elements as weak reference

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Steve Chung [:steveck]]

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 1

[Steve Chung [:steveck]]

Attachment: Bug 1348757 - Apply weakref for the element in cache.

Review commit: https://reviewboard.mozilla.org/r/122286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122286/

Comment 2

[Matthew N. [:MattN]]

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`

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8849499 [details]
Bug 1348757 - Apply weakref for the element in cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122286/diff/1-2/

Comment 4

[Steve Chung [:steveck]]

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 5

[Matthew N. [:MattN]]

Comment on attachment 8849499 [details]
Bug 1348757 - Apply weakref for the element in cache.

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

Comment 6

[Steve Chung [:steveck]]

Thanks!

Comment 7

[Iris Hsiao [:ihsiao]]

pushed: https://hg.mozilla.org/integration/autoland/rev/495a39e11056e7137b3b82e83e677d4c60994624

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/495a39e11056