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.
Created attachment 572174 [details] [diff] [review] Quick fix (thanks dolske) If it needs corrections, I'm hoping that they're only in coding conventions.
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?
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://firstname.lastname@example.org
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.)
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!
Comment on attachment 602563 [details] [diff] [review] Patch v.2 scram :P
Attachment #602563 - Flags: review-
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.
6 years ago
Attachment #615208 - Flags: review?(mnoorenberghe+bmo) → review+
Target Milestone: --- → mozilla15
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.