Closed
Bug 380931
Opened 18 years ago
Closed 17 years ago
Post-landing cleanup from Bug 374723 (rewrite password manager in JS)
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: Dolske, Assigned: Dolske)
References
Details
Attachments
(5 files, 2 obsolete files)
36.76 KB,
patch
|
mconnor
:
review+
sayrer
:
review+
|
Details | Diff | Splinter Review |
4.92 KB,
patch
|
Details | Diff | Splinter Review | |
16.58 KB,
patch
|
Details | Diff | Splinter Review | |
1.24 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
9.86 KB,
patch
|
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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 2•18 years ago
|
||
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 3•18 years ago
|
||
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+
Assignee | ||
Comment 4•18 years ago
|
||
Use nsIFile correctly instead of string concatenation.
Assignee | ||
Updated•18 years ago
|
Attachment #265468 -
Attachment description: Clean up storage's .initWithFile and tests → [done] Clean up storage's .initWithFile and tests
Assignee | ||
Updated•18 years ago
|
Attachment #266239 -
Attachment description: Bustage fix for previous patch → [done] Bustage fix for previous patch
Assignee | ||
Comment 5•18 years ago
|
||
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 6•18 years ago
|
||
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+
Comment 7•18 years ago
|
||
> We really need that toolkit prefs js file at some point.
This isn't happening, so you know -- see bug 231176.
Assignee | ||
Comment 8•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 9•18 years ago
|
||
mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 1.2
mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 1.5
Whiteboard: [checkin needed]
Assignee | ||
Comment 10•18 years ago
|
||
Oops, left this out of the last patch.
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 11•18 years ago
|
||
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%
Assignee | ||
Comment 12•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #266544 -
Attachment description: Clean up nsIFile usage, final. → [done] Clean up nsIFile usage, final.
Assignee | ||
Updated•18 years ago
|
Attachment #266573 -
Attachment description: Clean up nsIFile usage (oops) → [done] Clean up nsIFile usage (oops)
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
Comment 13•18 years ago
|
||
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 14•18 years ago
|
||
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)
Updated•18 years ago
|
Whiteboard: [checkin needed]
Updated•18 years ago
|
Attachment #266573 -
Attachment description: Clean up nsIFile usage (oops) → move prefs from firefox.js to all.js
Attachment #266573 -
Flags: review+
Comment 15•18 years ago
|
||
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
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 16•18 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
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
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #279867 -
Flags: review?(gavin.sharp)
Comment 19•17 years ago
|
||
The Target milestone here is outdated. can someone please update it?
Assignee | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 alpha6 → Firefox 3 M9
Updated•17 years ago
|
Attachment #279867 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 20•17 years ago
|
||
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
Assignee | ||
Comment 21•17 years ago
|
||
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?
Updated•17 years ago
|
Attachment #289409 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 22•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
Updated•17 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•