Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scanner: discard Form instances with no classified inputs #574

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

sjbarag
Copy link
Contributor

@sjbarag sjbarag commented May 23, 2024

Reviewer: @GioSensation (while @shakyShane is unavailable)
Asana: https://app.asana.com/0/1206682621538333/1207230997693280/f

Description

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.

Steps to test

  1. npm run test
    • Note the change in submit button statistics:
      Category Count Percent
      False Positive: Before 638 47%
      False Positive: After 596 45%
      False Negative: Before 389 14%
      False Negative: After 383 15%
  2. With these changes applied in either the browser extension or DDG browser, load https://www.scanmalta.com/shop (a page with currently a lot of <form/> elements).
  3. Click "Hello, sign in" at the top (on a desktop-shaped browser) and ensure the email/password inputs are properly identified with data-ddg-inputtype="credentials.username" and data-ddg-inputtype="credentials.password.current" attributes

@sjbarag sjbarag requested a review from GioSensation May 23, 2024 18:51
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.
@sjbarag sjbarag force-pushed the sbarag/ignore-small-entirely-unknown-forms branch from e28474f to b9c8959 Compare May 23, 2024 20:11
Copy link
Collaborator

@dbajpeyi dbajpeyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good to me, and tests are good too. Tested locally and the change works too! Merging it.

@dbajpeyi dbajpeyi merged commit 622c161 into main Jul 2, 2024
1 check passed
@dbajpeyi dbajpeyi deleted the sbarag/ignore-small-entirely-unknown-forms branch July 2, 2024 11:00
CDRussell pushed a commit to duckduckgo/Android that referenced this pull request Aug 2, 2024
Task/Issue URL:
https://app.asana.com/0/1207920271697429/1207920271697429
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.1.0


## Description
Updates Autofill to version
[12.1.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.1.0).

### Autofill 12.1.0 release notes
## What's Changed
* [Form] Skip autofilling select input if it was changed already by
@dbajpeyi in duckduckgo/duckduckgo-autofill#580
* scanner: discard Form instances with no classified inputs by @sjbarag
in duckduckgo/duckduckgo-autofill#574
* [Credentials] Autofill when elements are in shadow by @dbajpeyi in
duckduckgo/duckduckgo-autofill#592
* Update password-related json files (2024-07-19) by @daxmobile in
duckduckgo/duckduckgo-autofill#607
* [Scanner] Don't stop scanner fully on max forms by @dbajpeyi in
duckduckgo/duckduckgo-autofill#617
* tests: remove spurious logging from unit tests by @sjbarag in
duckduckgo/duckduckgo-autofill#575
* Add credit card test form failure by @GioSensation in
duckduckgo/duckduckgo-autofill#588

## New Contributors
* @dbajpeyi made their first contribution in
duckduckgo/duckduckgo-autofill#580

**Full Changelog**:
duckduckgo/duckduckgo-autofill@12.0.1...12.1.0

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

Co-authored-by: GioSensation <1828326+GioSensation@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants