Closed Bug 380931 Opened 14 years ago Closed 14 years ago

Post-landing cleanup from Bug 374723 (rewrite password manager in JS)

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(5 files, 2 obsolete files)

A few non-critical tasks were deferred from bug 374723, in order to land it on a tight deadline. Mostly review comments concerning code that works, but should be implemented a bit differently.

* remove doesFileExist() in favor of nsIFile.exists()
* expose signon.debug as a visible pref with a default value (|false|, natch).
* LoginManagerPrompter's _debug flag should be controlled by signon.debug
* test_storage_legacy_2.js was disabled because it had a hardcoded path to the reference files
* test_storage_legacy_1.js had one test disabled that should be enabled or removed
* The IE Profile Migrator was disabled due to a last-minute problem with it not being able to use the internal string API
* storage-Legacy should reuse a nsIFile object instead of creating it each time
* storage-Legacy should use nsIFile where needed instead of path/file name strings
* Login Manager should be started from a category in nsHTMLFormElement.cpp (like the old password manager), instead of being launched at startup from browser.js.
* Gavin suggested using String.localCompare in a few places
* "@mozilla.org/login-manager/authpromptfactory;1" had to revert in name to "@mozilla.org/passwordmanager/authpromptfactory;1"
* Convert "XXX" to bugs as needed
* Look at removing pref observers if they're not really needed (eg, for _debug)

Issues discovered after landing should be filed as separate bugs.
This changes .initWithFile to take nsIFile args instead of strings. The unit tests for the storage module were updated for this and cleaned up. Some tests that had been disabled because of limitations with the string args have now been reenabled.
Attachment #265468 - Flags: review?(gavin.sharp)
Comment on attachment 265468 [details] [diff] [review]
[done] Clean up storage's .initWithFile and tests

r=me on the code pieces, didn't dig so deep on the tests, but those look good too.
Attachment #265468 - Flags: review?(gavin.sharp) → review+
Comment on attachment 265468 [details] [diff] [review]
[done] Clean up storage's .initWithFile and tests

tests look ok to me. Could share more code in the head_ file.
Attachment #265468 - Flags: review+
Use nsIFile correctly instead of string concatenation.
Attachment #265468 - Attachment description: Clean up storage's .initWithFile and tests → [done] Clean up storage's .initWithFile and tests
Attachment #266239 - Attachment description: Bustage fix for previous patch → [done] Bustage fix for previous patch
Attached patch Clean up nsIFile usage (obsolete) — Splinter Review
Patch to clean up a few things...
* Made the signon.debug have a default value (no longer hidden)
* Get rid of the _datafile and _datapath strings in favor of a nsILocalFile
* Stop creating a nsILocalFile every call to _readFile() / _writeFile().

I also spun out part of init() into _getSignonsFile(). It makes init() a bit more readable, but makes this diff a little less readable in this spot.
Attachment #266444 - Flags: review?(mconnor)
Comment on attachment 266444 [details] [diff] [review]
Clean up nsIFile usage

>Index: browser/app/profile/firefox.js

>+pref("signon.debug",                        false); // logs to Error Console

you'll need to add this to all.js (or all of the other apps that use toolkit pwmgr) so other toolkit consumers don't throw like crazy.

We really need that toolkit prefs js file at some point.
Attachment #266444 - Flags: review?(mconnor) → review+
> We really need that toolkit prefs js file at some point.

This isn't happening, so you know -- see bug 231176.
Per mconnor on IRC, went ahead an moved all the signon prefs from firefox.js to all.js. Rest of the patch is unchanged.
Attachment #266444 - Attachment is obsolete: true
Whiteboard: [checkin needed]
mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 	1.2
mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 	1.5
Whiteboard: [checkin needed]
Oops, left this out of the last patch.
Whiteboard: [checkin needed]
This seems to have caused an increase in Rlk. When this landed without the default pref changes the leaks went up to 212Kb - landing the default prefs brought it back down to 3.09Kb, which is still higher than the original baseline of 2.91Kb. The landing of the default prefs stopped leaking 100% of most objects caused by the initial landing, except for the following (percentages are decreases in leaked objects due to the default pref landing):
XPCNativeScriptableShared                       216     -33.33%
XPCWrappedNativeProto                           672      -4.00%
nsLocalFile                                     240     -77.78%
nsPrefBranch                                     56     -50.00%
nsStringBuffer                                   40     -99.26%
nsTArray_base                                     4     -95.24%
nsVoidArray                                       4     -99.15%
nsXPCComponents                                  60     -96.00%
Checkpoint as to remaining items from comment #0:

* LoginManagerPrompter's _debug flag should be controlled by signon.debug
* Login Manager should be started from a category in nsHTMLFormElement.cpp
(like the old password manager), instead of being launched at startup from
browser.js.
* Gavin suggested using String.localCompare in a few places
* "@mozilla.org/login-manager/authpromptfactory;1" had to revert in name to
"@mozilla.org/passwordmanager/authpromptfactory;1"
* Convert "XXX" to bugs as needed
* Look at removing pref observers if they're not really needed (eg, for _debug)
Attachment #266544 - Attachment description: Clean up nsIFile usage, final. → [done] Clean up nsIFile usage, final.
Attachment #266573 - Attachment description: Clean up nsIFile usage (oops) → [done] Clean up nsIFile usage (oops)
Whiteboard: [checkin needed]
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
Here is a diff of the leak logs from 2.91 KB -> 3.09 KB:

--NEW-LEAKS-----------------------------------leaks------leaks%
nsLocalFile                                     240     100.00%
nsStringBuffer                                   40      25.00%
XPCWrappedNative                               1568       3.70%

and the bonsai URL for good measure: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-29+19%3A44&maxdate=1180496759&cvsroot=%2Fcvsroot
Comment on attachment 266573 [details] [diff] [review]
[done] move prefs from firefox.js to all.js

This hasn't been checked in yet.
Attachment #266573 - Attachment description: [done] Clean up nsIFile usage (oops) → Clean up nsIFile usage (oops)
Whiteboard: [checkin needed]
Attachment #266573 - Attachment description: Clean up nsIFile usage (oops) → move prefs from firefox.js to all.js
Attachment #266573 - Flags: review+
Comment on attachment 266573 [details] [diff] [review]
[done] move prefs from firefox.js to all.js

browser/app/profile/firefox.js 1.182
modules/libpref/src/init/all.js 3.673
Attachment #266573 - Attachment description: move prefs from firefox.js to all.js → [done] move prefs from firefox.js to all.js
Whiteboard: [checkin needed]
Blocks: 304309
Over in bug 385237, I figured out that the cause of a content pref service leak is the nsIFactory that creates the service.  Some testing leads me to conclude that the factories in every JS component are likely to be leaking, which may be the cause of (some of) the leaks reported in the bugs to which I am adding this comment (bug 337050, bug 378618, bug 381239, and bug 380873/bug 380931).

Take a look at bug 385237, comment 2 for more details, and note that the fix for bug 180380 makes all the XPCWrappedNative and XPCWrappedNativeProto leaks (which were what the content pref service was leaking) go away and may similarly fix (some of) the leaks reported in this bug.
Updates since comment #12:

* The leaks are gone (other than the issue in bug 384230).
* Moved the ""@mozilla.org/passwordmanager/authpromptfactory;1" issue over to bug 380917, since it's blocked on other things.
* Opened 394603 for the "start from a category" issue
* Opened 394604 for the "String.localCompare" issue.
* I think the current pref observers are fine as-is, and there's not much point in thinking about trying to remove them.

An upcoming patch will fix:

* LoginManagerPrompter's _debug flag should be controlled by signon.debug
* Convert "XXX" to bugs as needed
Attached patch Remove XXXs, get debug pref (obsolete) — Splinter Review
Attachment #279867 - Flags: review?(gavin.sharp)
The Target milestone here is outdated.  can someone please update it?
Target Milestone: Firefox 3 alpha6 → Firefox 3 M9
Attachment #279867 - Flags: review?(gavin.sharp) → review+
Updated patch to trunk. Will file a bug for one of the XXX issues removed, but the reset were good.
Attachment #279867 - Attachment is obsolete: true
Comment on attachment 289409 [details] [diff] [review]
Remove XXXs, get debug pref v.2

a1.9? Very low impact, almost entirely just comment changes.
Attachment #289409 - Flags: approval1.9?
Attachment #289409 - Flags: approval1.9? → approval1.9+
Finally done! Yay!

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.17; previous revision: 1.16
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Product: Firefox → Toolkit
Depends on: 433070
You need to log in before you can comment on or make changes to this bug.