412381 - Clear Private Data should delete old signons.txt file(s).

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

We've upgraded from signons.txt to signons2.txt and now to signons3.txt. Data is automatically imported from old files, but the old files are not removed.

Clear private data calls nsILoginManager.removeAllLogins(), which should go ahead and nuke any old files.

Related: Bug 329741 discusses deleting other files based on last-modified times.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress

Comment 2

[Reed Loden [:reed]]

Comment on attachment 301358 [details] [diff] [review]
Patch, work in progress

>+     * Deletes any stoage files that we're not using any more.

s/stoage/storage/

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Wanted, not blocking.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 305706 [details] [diff] [review]
Patch v.2

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+    _removeOldSignonsFiles : function() {
>+        // Get the location of the user's profile.
>+        var DIR_SERVICE = new Components.Constructor(
>+                "@mozilla.org/file/directory_service;1", "nsIProperties");
>+        var pathname = (new DIR_SERVICE()).get("ProfD", Ci.nsIFile).path;
>+
>+        // We've used a number of prefs over time due to compatibility issues.
>+        // Skip the first entry (the newest) and delete the others.
>+        for (var i = 1; i < this._filenamePrefs.length; i++) {
>+            var prefName = this._filenamePrefs[i];
>+
>+            var filename = this._prefBranch.getCharPref(prefName);
>+
>+            var file = Cc["@mozilla.org/file/local;1"].
>+                       createInstance(Ci.nsILocalFile);
>+            file.initWithPath(pathname);
>+            file.append(filename);

I didn't notice this when I reviewed the original code that this is copied from, but there's no point in using a Constructor when you only get the service once, and you could keep a reference to the profile's nsIFile and clone it each time rather than keeping the pathName and calling createInstance/initWithPath each time. Could also move that logic into a getFileFromPref function and share this code with _getSignonsFile?

Otherwise this looks fine.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305869 [details] [diff] [review]
Patch v.3

a=beltzner for 1.9

Comment 8

[Justin Dolske [:Dolske]]

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.24; previous revision: 1.23
Checking in toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js;
  new revision: 1.10; previous revision: 1.9
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_2.js;
  new revision: 1.6; previous revision: 1.5