Limit form parsing to first + last N forms in a document.

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Password Manager
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
I was just watching pwmgr debug output while browsing, and noticed that loading a comment thread on Reddit results in a bunch of spew. Not sure it it's always been this way, but apparently each comment is wrapped in a <form>, as is each "permalink/report/reply" footer for a comment. End result it that on pageload pwmgr tries wading through 1000+ (!) forms looking for a possible login form.

Unbounded checking like this can result in apparent browser hangs in pathological cases like this.

We should only check the first and last few forms in a page. Say, the first and last 20 forms. When a page has hundreds or thousands of forms, it seems highly unlikely that a form in the middle would be the login form.

Fix should be simple: In /toolkit/components/passwordmgr/nsLoginManager.js, look for _fillDocument() and the "for (var i = 0; i < forms.length; i++)" loop. Limit this as above.

Would be good to add tests to ensure we skip the middle form in a document with 41 forms, as well as ensuring we will fill the 20th and 42nd form. I'm happy to help with this part.

Bonus point for doing the same for onFormSubmit, and checking satchel's behavior wrt to this.
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug]

Comment 1

7 years ago
Created attachment 572174 [details] [diff] [review]
Quick fix (thanks dolske)

If it needs corrections, I'm hoping that they're only in coding conventions.
(Assignee)

Comment 2

7 years ago
Bah, I thought I had already commented on this:

This will make some existing tests fail (notably toolkit/components/passwordmgr/test/test_basic_form*) because they just happen to cram a lot of tests into a single page. The value of (at least some) of these tests it low, especially the perl-generated ones dating back to the Firefox 3 pwmgr rewrite. They could be replaced by a set of more concise tests.

So that will need to happen before this can land.
Pushed this to try to find out what tests will need to be fixed:
https://tbpl.mozilla.org/?tree=Try&rev=a9842f3187c3

Alex, do you need help on how to write a new test for this, as dolske mentioned in comment 0?

Comment 4

7 years ago
Try run for a9842f3187c3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a9842f3187c3
Results (out of 209 total builds):
    exception: 1
    success: 160
    warnings: 34
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/felipc@gmail.com-a9842f3187c3
(Assignee)

Comment 6

6 years ago
Created attachment 602563 [details] [diff] [review]
Patch v.2

So I finally rewrote the tests. And then added new tests for the code added here. And then decided the code should be different so that it's a little more robust/foolproof.

Hate to yoink the bug away from you Alex, but I guess I already replaced everything. Happy to help with another [good first bug] if you're interested!
Assignee: nobody → dolske
Attachment #572174 - Attachment is obsolete: true
Attachment #572174 - Flags: review?(dolske)
Attachment #602563 - Flags: review?(mnoorenberghe+bmo)
(Assignee)

Updated

6 years ago
Whiteboard: [good first bug]
Comment on attachment 602563 [details] [diff] [review]
Patch v.2

Review of attachment 602563 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +1074,5 @@
>          var foundLogins = null;
>  
> +        // Limit the number of forms we try to fill. If there are too many
> +        // forms, just fill a few at the beginning of page and a few at the
> +        // end of the page.

Nit: s/a few/some/.  Can be simplified: "...just fill some at the beginning and end of the document"

@@ +1075,5 @@
>  
> +        // Limit the number of forms we try to fill. If there are too many
> +        // forms, just fill a few at the beginning of page and a few at the
> +        // end of the page.
> +        var MAX_FORMS = 40; // assumed to be an even number

Make this a const

@@ +1078,5 @@
> +        // end of the page.
> +        var MAX_FORMS = 40; // assumed to be an even number
> +        var skip_from = -1, skip_to = -1;
> +        if (forms.length > MAX_FORMS) {
> +            this.log("fillDocument limiting number of formfills to " + MAX_FORMS);

NIT: "formfills" can be confused with satchel (browser.formfill.*).  How about "forms filled".

@@ +1088,2 @@
>          for (var i = 0; i < forms.length; i++) {
> +            // Skip some in the middle of the doc if there were too many.

Nit: s/doc/document/

::: toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html
@@ +2,4 @@
>  <html>
>  <head>
>    <title>Test for Login Manager</title>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  

Nit: Might as well be consistent and kill the trailing whitespace in this file too.

::: toolkit/components/passwordmgr/test/test_maxforms_3.html
@@ +37,5 @@
> +commonInit();
> +/** Test for Login Manager: form fill, multiple forms. **/
> +
> +function startTest() {
> +  for (var i = 1; i < FORMS_TO_CREATE; i++) {

itym "<=" since it's a 1-based array.  Same for all 3 new tests.

@@ +47,5 @@
> +      is($_(i, "p").value, "", "Checking for unfilled password in form " + i);
> +    }
> +  }
> +
> +  SimpleTest.finish();

We should add more checks to the skipped form: 
* If the user types their username and then blurs the field the password is filled in.
* Autocomplete dropdown still shows on username field after typing prefix.
* Autocomplete dropdown still shows on username field with down arrow.
Attachment #602563 - Flags: review?(mnoorenberghe+bmo)
(Assignee)

Comment 9

6 years ago
Created attachment 615208 [details] [diff] [review]
Patch v.3

Fixed nits from comment 7.

Didn't change the last bit, though. We don't hoop up the blur/autocomplete, so form fill won't work that way... That will require far more invasive changes, and this should (hopefully) all be changing soon when we make pwmgr deal with script-generated forms, so I'm leaving as-is.
Attachment #602563 - Attachment is obsolete: true
Attachment #615208 - Flags: review?(mnoorenberghe+bmo)
Attachment #615208 - Flags: review?(mnoorenberghe+bmo) → review+
(Assignee)

Comment 10

6 years ago
Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/ac0354359a8c
Target Milestone: --- → mozilla15
http://hg.mozilla.org/mozilla-central/rev/ac0354359a8c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.