Closed
Bug 699707
Opened 13 years ago
Closed 12 years ago
Limit form parsing to first + last N forms in a document.
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: Dolske, Assigned: Dolske)
Details
Attachments
(1 file, 2 obsolete files)
654.66 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Whiteboard: [good first bug]
Comment 1•13 years ago
|
||
If it needs corrections, I'm hoping that they're only in coding conventions.
Updated•13 years ago
|
Attachment #572174 -
Flags: review?(dolske)
Assignee | ||
Comment 2•12 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.
Comment 3•12 years ago
|
||
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•12 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
Comment 5•12 years ago
|
||
The two test files that fails with this patch are: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_basic_form_1pw.html?force=1 and http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html?force=1 (These are the two generated by perl scripts.)
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
Whiteboard: [good first bug]
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
Comment on attachment 602563 [details] [diff] [review] Patch v.2 scram :P
Attachment #602563 -
Flags: review-
Assignee | ||
Comment 9•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #615208 -
Flags: review?(mnoorenberghe+bmo) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/ac0354359a8c
Target Milestone: --- → mozilla15
Comment 11•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ac0354359a8c
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•