Re-enable mutObs with better safeguards #403

GioSensation · 2023-10-25T15:57:56Z

Reviewer: @shakyShane
Asana: https://app.asana.com/0/0/1205688498594652/f

Description

Re-enables the mutation observer within the form, with some critical changes highlighted inline. There's a lot more info on what I tried out and what I discarded and why in the task.

Steps to test

Re-enabled the automated tests + followed the steps in the parent task.

Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation · 2023-10-25T16:15:16Z

integration-test/helpers/harness.js

@@ -41,7 +41,7 @@ export async function withEmailProtectionExtensionSignedInAs (page, username) {
 *     MAX_INPUTS_PER_PAGE: number,
 *     MAX_FORMS_PER_PAGE: number,
 *     MAX_INPUTS_PER_FORM: number,
- *     MAX_FORM_MUT_OBS_COUNT: number
+ *     MAX_FORM_RESCANS: number


Renamed to capture the fact that the limit is per scan, regardless of whether the mutation observer initiated the rescan. This was at the heart of the issue we were seeing, where the form could rescan the same forms over and over without running into a failsafe.

Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation · 2023-10-25T16:23:12Z

src/Form/Form.js

+                    // Ensure we destroy the form if it's removed from the DOM
+                    if (!this.form.isConnected) {
+                        this.destroy()
+                        return
+                    }


The mutation observer callback is mostly the same as it was. This little bit is new. Just ensures we're cleaning things up and don't run on disconnected forms. Extremely unlikely, but there you go.

GioSensation · 2023-10-25T16:24:36Z

src/Form/Form.js

+                    // Must check for inputs because a parent may be removed and not show up in record.removedNodes
+                    if ([...this.inputs.all].some(input => !input.isConnected)) {
+                        // This is re-connected in recategorizeAllInputs, disconnecting here to avoid risk of re-work
+                        this.mutObs.disconnect()


This is also new. Since the re-scans happen in an idleCallback, I make sure to disconnect the observer here to avoid any risk of the callback running multiple times in a row. Extreme edge case, but there you go 🙂.

GioSensation · 2023-10-25T16:26:54Z

src/Form/Form.js

+        // If the form mutates too much, disconnect to avoid performance issues
+        if (this.rescanCount >= MAX_FORM_RESCANS) {
+            this.mutObs.disconnect()
+            return
+        }
+        this.rescanCount++


This is the critical part of the fix. In the previous implementation, the counter was only updated within the observer callback, but the recategorizeAllInputs could also be called from addInput coming from the scanner. That was bypassing the failsafes and causing it to spin out of control in certain edge cases. Now the count is incremented right within the method so it's never missed 💪🤞.

GioSensation · 2023-10-25T16:27:45Z

src/Form/Form.js

        this.removeAllDecorations()
        this.removeTooltip()
        this.forgetAllInputs()
        this.matching.clear()
        this.intObs = null
+        this.device.scanner.forms.delete(this.form)


This is also new. When destroying the form we also ensure not to keep track of it in the scanner ledger. I don't particularly like reaching out to the scanner, but it's effective.

GioSensation · 2023-10-25T16:29:42Z

src/Form/Form.js

+                // This is rather extreme, but better safe than sorry
+                this.device.scanner.stopScanner('The form has too many inputs, bailing.')
+                return


This is another crucial part. Previously, the else block would not return nor stop the scanner, thus a lot of work would keep happening after hitting the failsafes. TBH, stopping the scanner completely is probably overkill, but we've been burned before so I'm happy to go nuclear here.

GioSensation · 2023-10-25T16:31:17Z

src/Form/Form.js

+        if (this.form !== document.body) {
+            this.mutObs.observe(this.form, this.mutObsConfig)
+        }


Another important part here. The problem happened specifically when autofill got tricked into thinking that the whole body was a form (for example, when there is an input field at the top of the page and a bunch of fields down below). This ensures that when that happens we don't append the observer at all. Below there are a couple other measures to ensure we decrease the chance of identifying the body as the form.

GioSensation · 2023-10-25T16:31:45Z

src/Form/Form.js

-                console.log('The form has too many inputs, destroying.')
-            }
-            this.destroy()
+            this.device.scanner.stopScanner('The form has too many inputs, destroying.')


Again, going nuclear. Instead of just destroying the form, we stop the scanner altogether.

GioSensation · 2023-10-25T16:32:42Z

src/Form/Form.js

+        }
+
+        // When new inputs are added after the initial scan, reanalyze the whole form
+        if (this.initialScanComplete && this.rescanCount < MAX_FORM_RESCANS) {


Checking the counter here as well, to avoid running the FormAnalyzer again, which is very expensive when the form doesn't have good markup.

GioSensation · 2023-10-25T16:35:33Z

src/Form/matching-config/selectors-css.js

+    genericTextField,
+    '[autocomplete=username]',
+    'select'
+]


I noticed that there was a lot of duplication and incomplete selectors between these two, so I've moved them closer, and derive one from the other to avoid duplication and one getting out of sync with the other. The meaningful change here is to add :not([placeholder^=search i]) which skips more search fields.

GioSensation · 2023-10-25T16:36:04Z

src/Form/test-cases/asana_search.html

+        <div class="TopbarPageHeaderGlobalActions-settingsMenuAvatar Avatar AvatarPhoto AvatarPhoto--small AvatarPhoto--color6"
+             role="img"
+             style="background-image: url(&quot;https://s3.amazonaws.com/profile_photos/1144596632862977.1144596633302696.AZv0WLh0YnXpeia8yd53_60x60.png&quot;);"></div>
+    </a></div>


Just reformatted while I was using to debug.

GioSensation · 2023-10-25T16:41:23Z

src/Scanner.js

        let element = input
        // traverse the DOM to search for related inputs
-        while (element.parentElement && element.parentElement !== document.documentElement) {
+        while (traversalLayerCount <= 5 && element.parentElement && element.parentElement !== document.documentElement) {


This is the last important bit. We limit the upward traversal when searching for a form container. This decreases the chance of false-matching the body as a form container. For example, in Asana, with a specific sequence of clicks the algo could have matched the whole body, thus running extremely expensive checks on the whole DOM. With this change the algo stops at the rows of tasks, as expected. This ensures we hit the max form limit much quicker and have more tightly-scoped observers.

shakyShane · 2023-10-30T09:58:58Z

thanks for the deep-dive and the inline annotations @GioSensation 🙏🏻

GioSensation added 7 commits October 19, 2023 14:47

Improve skipping search forms

22becb6

Signed-off-by: Emanuele Feliziani <[email protected]>

Limit the upward traversal when searching the form

30373b7

Signed-off-by: Emanuele Feliziani <[email protected]>

Fix type

e6c4970

Signed-off-by: Emanuele Feliziani <[email protected]>

Re-enable the mutation observer within the form

3e8d9a8

Signed-off-by: Emanuele Feliziani <[email protected]>

Rename constant

ccc5a34

Signed-off-by: Emanuele Feliziani <[email protected]>

Add comment

f592a85

Signed-off-by: Emanuele Feliziani <[email protected]>

Add compiled files

0e03a44

Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation self-assigned this Oct 25, 2023

Fix name in test harness

7b89b5e

Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation commented Oct 25, 2023

View reviewed changes

Add comment

f7cf4d4

Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation commented Oct 25, 2023

View reviewed changes

GioSensation changed the title ~~[WIP] Re-enable mutObs with better safeguards~~ Re-enable mutObs with better safeguards Oct 25, 2023

GioSensation requested a review from shakyShane October 25, 2023 16:43

GioSensation marked this pull request as ready for review October 25, 2023 16:43

shakyShane approved these changes Oct 30, 2023

View reviewed changes

GioSensation merged commit e9c8133 into main Oct 31, 2023
1 check passed

GioSensation deleted the ema/fix-algo-spiralling-out-of-control branch October 31, 2023 07:48

daxmobile mentioned this pull request Oct 31, 2023

Update autofill to 9.1.0 duckduckgo/BrowserServicesKit#545

Closed

This was referenced Oct 31, 2023

Update autofill to 9.1.0 duckduckgo/Android#3778

Closed

Update BSK with autofill 9.1.0 duckduckgo/macos-browser#1789

Closed

Update BSK with autofill 9.1.0 duckduckgo/iOS#2119

Closed

Update autofill to 9.1.0 duckduckgo/duckduckgo-privacy-extension#2310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable mutObs with better safeguards #403

Re-enable mutObs with better safeguards #403

GioSensation commented Oct 25, 2023 •

edited

Loading

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

GioSensation Oct 25, 2023

shakyShane commented Oct 30, 2023

Re-enable mutObs with better safeguards #403

Re-enable mutObs with better safeguards #403

Conversation

GioSensation commented Oct 25, 2023 • edited Loading

Description

Steps to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakyShane commented Oct 30, 2023

GioSensation commented Oct 25, 2023 •

edited

Loading