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

RESOLVED FIXED in mozilla1.9beta1

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla1.9beta1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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

12 years ago
Created attachment 265468 [details] [diff] [review]
[done] Clean up storage's .initWithFile and tests

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 3

12 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

12 years ago
Created attachment 266239 [details] [diff] [review]
[done] Bustage fix for previous patch

Use nsIFile correctly instead of string concatenation.
(Assignee)

Updated

12 years ago
Attachment #265468 - Attachment description: Clean up storage's .initWithFile and tests → [done] Clean up storage's .initWithFile and tests
(Assignee)

Updated

12 years ago
Attachment #266239 - Attachment description: Bustage fix for previous patch → [done] Bustage fix for previous patch
(Assignee)

Comment 5

12 years ago
Created attachment 266444 [details] [diff] [review]
Clean up nsIFile usage

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+

Comment 7

12 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

12 years ago
Created attachment 266544 [details] [diff] [review]
[done] Clean up nsIFile usage, final.

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

12 years ago
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]
(Assignee)

Comment 10

12 years ago
Created attachment 266573 [details] [diff] [review]
[done] move prefs from firefox.js to all.js

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%
(Assignee)

Comment 12

12 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

12 years ago
Attachment #266544 - Attachment description: Clean up nsIFile usage, final. → [done] Clean up nsIFile usage, final.
(Assignee)

Updated

12 years ago
Attachment #266573 - Attachment description: Clean up nsIFile usage (oops) → [done] Clean up nsIFile usage (oops)
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6

Comment 13

12 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 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]

Updated

12 years ago
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.
(Assignee)

Comment 17

11 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

11 years ago
Created attachment 279867 [details] [diff] [review]
Remove XXXs, get debug pref
Attachment #279867 - Flags: review?(gavin.sharp)

Comment 19

11 years ago
The Target milestone here is outdated.  can someone please update it?
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 alpha6 → Firefox 3 M9
Attachment #279867 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 20

11 years ago
Created attachment 289409 [details] [diff] [review]
Remove XXXs, get debug pref v.2

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

11 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

11 years ago
Attachment #289409 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 22

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
Product: Firefox → Toolkit
(Assignee)

Updated

10 years ago
Depends on: 433070
You need to log in before you can comment on or make changes to this bug.