Closed
Bug 1348757
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Cache input elements as weak reference
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: steveck, Assigned: steveck)
References
Details
Attachments
(1 file)
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 2•7 years ago
|
||
mozreview-review |
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 years 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 5•7 years ago
|
||
mozreview-review |
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+
Comment 7•7 years ago
|
||
pushed: https://hg.mozilla.org/integration/autoland/rev/495a39e11056e7137b3b82e83e677d4c60994624
Keywords: checkin-needed
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/495a39e11056
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•