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.

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.

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.

Use nsIFile correctly instead of string concatenation.

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.

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.

> 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.

mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 1.2 mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 1.5

Oops, left this out of the last patch.

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)

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.

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

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

The Target milestone here is outdated. can someone please update it?

Updated patch to trunk. Will file a bug for one of the XXX issues removed, but the reset were good.

Comment on attachment 289409 [details] [diff] [review] Remove XXXs, get debug pref v.2 a1.9? Very low impact, almost entirely just comment changes.

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