scanner: discard Form instances with no classified inputs (#574)

When encountering <form/>s with no fillable inputs, the Scanner and Form classes previously maintained MutationObservers for each <form/>. This allowed the global MutationObserver to detect newly-added <input/>s and attribute them to existing Form instances without having to create a new Form, but also meant we maintained observers and listeners that often went unused. For performance reasons, we stop processing <form/> elements once 30 Form instances have been created. This allowed a large number of unfillable <form/>s could consume the entire Form pool before an actually-fillable <form/> is encountered. Immediately `destroy()` a Form instance that contains no fillable <input/>s, and avoid adding that instance to a Scanner's set of known Form instances. This ensures form-scoped listeners and observers are only maintained when a fillable field is present. When a new <input/> is added the global MutationObserver and Scanner create a new Form instance to manage it, enabling slightly lazier listener/observer initialization.
duckduckgo · Jul 2, 2024 · 622c161 · 622c161
1 parent 7d6a5fd
commit 622c161
Show file tree

Hide file tree

Showing 18 changed files with 175 additions and 32 deletions.
diff --git a/dist/autofill-debug.js b/dist/autofill-debug.js
diff --git a/dist/autofill.js b/dist/autofill.js
diff --git a/integration-test/pages/scanner-perf.html b/integration-test/pages/scanner-perf.html
@@ -13,6 +13,8 @@
 <body>
 <div>
     <form action="" id="demo" name="demo">
+        <!-- Add one classifiable field, to keep this form from being immediately disposed. -->
+        <label>Email: <input name="email" type="email"/></label>
         <button type="submit">Sign in</button>
     </form>
 </div>

diff --git a/src/Form/Form.js b/src/Form/Form.js
@@ -52,6 +52,7 @@ class Form {
      * @param {Boolean} [shouldAutoprompt]
      */
     constructor (form, input, deviceInterface, matching, shouldAutoprompt = false) {
+        this.destroyed = false
         this.form = form
         this.matching = matching || createMatching()
         this.formAnalyzer = new FormAnalyzer(form, input, matching)
@@ -114,11 +115,28 @@ class Form {
 
         this.logFormInfo()
 
+        const numKnownInputs = this.inputs.all.size - this.inputs.unknown.size
+        if (numKnownInputs === 0) {
+            // This form has too few known inputs and likely doesn't make sense
+            // to track for autofill (e.g. a single-input for search or item quantity).
+            // Self-destruct to stop listening and avoid memory leaks.
+            if (shouldLog()) {
+                console.log(`Form discarded: zero inputs are fillable`)
+            }
+            this.destroy()
+            return
+        }
+
         if (shouldAutoprompt) {
             this.promptLoginIfNeeded()
         }
     }
 
+    /** Whether this form has been destroyed via the `destroy` method or not. */
+    get isDestroyed () {
+        return this.destroyed
+    }
+
     get isLogin () {
         return this.formAnalyzer.isLogin
     }
@@ -363,6 +381,7 @@ class Form {
     }
     // This removes all listeners to avoid memory leaks and weird behaviours
     destroy () {
+        this.destroyed = true
         this.mutObs.disconnect()
         this.removeAllDecorations()
         this.removeTooltip()

diff --git a/src/Form/Form.test.js b/src/Form/Form.test.js
@@ -414,4 +414,20 @@ describe('Form bails', () => {
         decoratedInputs = document.querySelectorAll(`[${constants.ATTR_INPUT_TYPE}]`)
         expect(decoratedInputs).toHaveLength(0)
     })
+
+    test('when not enough fields have been classified on load', async () => {
+        const formEl = attachAndReturnGenericForm(`
+<form>
+    <!-- A hidden field for the product number -->
+    <input type="hidden" name="sku" value="lorem"/>
+    <!-- A visible field for the number to purchase -->
+    <input type="number" name="qty" title="Qty"/>
+</form>
+        `)
+        const scanner = createScanner(InterfacePrototype.default()).findEligibleInputs(document)
+        const formClass = scanner.forms.get(formEl)
+        expect(formClass).not.toBeDefined()
+        const decoratedInputs = document.querySelectorAll(`[${constants.ATTR_INPUT_TYPE}]`)
+        expect(decoratedInputs).toHaveLength(0)
+    })
 })
diff --git a/src/Form/input-classifiers.test.js b/src/Form/input-classifiers.test.js
@@ -212,12 +212,14 @@ describe.each(testCases)('Test $html fields', (testCase) => {
          */
         const identifiedSubmitButtons = Array.from(document.querySelectorAll('[data-manual-submit]'))
 
-        let submitFalsePositives = detectedSubmitButtons.filter(button => !identifiedSubmitButtons.includes(button)).length
-        let submitFalseNegatives = identifiedSubmitButtons.filter(button => !detectedSubmitButtons.includes(button)).length
+        // False positives are tracked in a Form instance but not marked with 'data-manual-submit' in the DOM.
+        let submitFalsePositives = detectedSubmitButtons.filter(button => !identifiedSubmitButtons.includes(button))
+        // False negatives are marked with 'data-manual-submit' in the DOM but not tracked in a Form instance.
+        let submitFalseNegatives = identifiedSubmitButtons.filter(button => !detectedSubmitButtons.includes(button))
 
         if (!generated) {
-            expect(submitFalsePositives).toEqual(expectedSubmitFalsePositives)
-            expect(submitFalseNegatives).toEqual(expectedSubmitFalseNegatives)
+            expect(submitFalsePositives).toHaveLength(expectedSubmitFalsePositives)
+            expect(submitFalseNegatives).toHaveLength(expectedSubmitFalseNegatives)
         }
 
         /** @type {Array<HTMLInputElement>} */
@@ -260,8 +262,8 @@ describe.each(testCases)('Test $html fields', (testCase) => {
         const submitButtonScores = {
             detected: detectedSubmitButtons.length,
             identified: identifiedSubmitButtons.length,
-            falsePositives: submitFalsePositives,
-            falseNegatives: submitFalseNegatives
+            falsePositives: submitFalsePositives.length,
+            falseNegatives: submitFalseNegatives.length
         }
 
         testResults.push({testCase, scores, submitButtonScores})

diff --git a/src/Scanner.js b/src/Scanner.js
@@ -298,7 +298,11 @@ class DefaultScanner {
 
             // Only add the form if below the limit of forms per page
             if (this.forms.size < this.options.maxFormsPerPage) {
-                this.forms.set(parentForm, new Form(parentForm, input, this.device, this.matching, this.shouldAutoprompt))
+                const f = new Form(parentForm, input, this.device, this.matching, this.shouldAutoprompt)
+                // Also only add the form if it hasn't self-destructed due to having too few fields
+                if (!f.isDestroyed) {
+                    this.forms.set(parentForm, f)
+                }
             } else {
                 this.stopScanner('The page has too many forms, stop adding them.')
             }

diff --git a/src/Scanner.test.js b/src/Scanner.test.js
@@ -6,8 +6,8 @@ describe('performance', () => {
         require('./requestIdleCallback.js')
         document.body.innerHTML = `
             <form action="">
-                <label for="input-01"><input type="text" id="input-01"></label>
-                <label for="input-02"><input type="text" id="input-02"></label>
+                <label for="input-01">Username<input type="text" id="input-01"></label>
+                <label for="input-02">Password<input type="password" id="input-02"></label>
                 <label for="input-03"><input type="text" id="input-03"></label>
                 <label for="input-04"><input type="text" id="input-04"></label>
                 <label for="input-05"><input type="text" id="input-05"></label>

diff --git a/src/__snapshots__/Scanner.test.js.snap b/src/__snapshots__/Scanner.test.js.snap
@@ -12,8 +12,9 @@ exports[`performance should constrain the buffer size 1`] = `
     <label
       for="input-01"
     >
+      Username
       <input
-        data-ddg-inputtype="unknown"
+        data-ddg-inputtype="credentials.username"
         id="input-01"
         type="text"
       />
@@ -23,10 +24,11 @@ exports[`performance should constrain the buffer size 1`] = `
     <label
       for="input-02"
     >
+      Password
       <input
-        data-ddg-inputtype="unknown"
+        data-ddg-inputtype="credentials.password.new"
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>
     
@@ -80,8 +82,9 @@ exports[`performance should debounce dom lookups 1`] = `
     <label
       for="input-01"
     >
+      Username
       <input
-        data-ddg-inputtype="unknown"
+        data-ddg-inputtype="credentials.username"
         id="input-01"
         type="text"
       />
@@ -91,10 +94,11 @@ exports[`performance should debounce dom lookups 1`] = `
     <label
       for="input-02"
     >
+      Password
       <input
-        data-ddg-inputtype="unknown"
+        data-ddg-inputtype="credentials.password.new"
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>
     
@@ -148,6 +152,7 @@ exports[`performance should not scan if above maximum inputs 1`] = `
     <label
       for="input-01"
     >
+      Username
       <input
         id="input-01"
         type="text"
@@ -158,9 +163,10 @@ exports[`performance should not scan if above maximum inputs 1`] = `
     <label
       for="input-02"
     >
+      Password
       <input
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>
     
@@ -211,6 +217,7 @@ exports[`performance should stop scanning if page grows above maximum forms 1`]
     <label
       for="input-01"
     >
+      Username
       <input
         id="input-01"
         type="text"
@@ -221,9 +228,10 @@ exports[`performance should stop scanning if page grows above maximum forms 1`]
     <label
       for="input-02"
     >
+      Password
       <input
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>
     
@@ -267,6 +275,7 @@ exports[`performance should stop scanning if page grows above maximum forms 1`]
     <label
       for="input-01"
     >
+      Username
       <input
         id="input-01"
         type="text"
@@ -277,9 +286,10 @@ exports[`performance should stop scanning if page grows above maximum forms 1`]
     <label
       for="input-02"
     >
+      Password
       <input
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>
     
@@ -330,6 +340,7 @@ exports[`performance should stop scanning if page grows above maximum inputs 1`]
     <label
       for="input-01"
     >
+      Username
       <input
         id="input-01"
         type="text"
@@ -340,9 +351,10 @@ exports[`performance should stop scanning if page grows above maximum inputs 1`]
     <label
       for="input-02"
     >
+      Password
       <input
         id="input-02"
-        type="text"
+        type="password"
       />
     </label>