Closed Bug 288040 Opened 19 years ago Closed 16 years ago

convert signons3.txt to mozstorage

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1a2

People

(Reporter: mikegoodspeed, Assigned: zpao)

References

Details

Attachments

(7 files, 23 obsolete files)

778 bytes, text/plain
Details
627 bytes, text/html
Details
5.80 KB, text/plain
Details
180.00 KB, application/x-tar
Details
90.27 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
67.28 KB, application/zip
Details
80.91 KB, patch
Details | Diff | Splinter Review
For my capstone I need to convert signons.txt in the profile data to use the new
SQLite db in MozStorage: http://wiki.mozilla.org/Mozilla2:Unified_Storage
11:51:45 AM - Simplex: things I need are like names of funtions, where it exists
in the code tree, and all that
11:52:03 AM - Vlad: though
http://lxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/base
11:52:08 AM - Vlad: is the stuff that deals with signons.txt
11:53:02 AM - Vlad: but what you basically need is to use
NS_NewLocalFileInputStream() to get an input stream to the file
11:53:18 AM - Vlad: take a look at nsPasswordManager::ReadPasswords
11:54:21 AM - Simplex: oh, wow.  that's some good stuff
11:56:37 AM - Simplex: can I get a spec for it
11:56:42 AM - Simplex: like a name and input params?
11:56:50 AM - Vlad: lxr is your friend
11:56:59 AM - Simplex: see, but I'm lost
11:57:02 AM - Simplex: it's huge
11:57:10 AM - Vlad: http://lxr.mozilla.org/seamonkey/ put on
NS_NewLocalFileInputStream in the text search
11:57:15 AM - Vlad: (or identifier search, but that's not always reliable)
11:57:37 AM - Vlad: then scroll through until you find something that looks like
the function declaration (in this case in nsNetUtil.h)
11:59:26 AM - Simplex: ok, I can figure all that out, I think.  but I'm talking
about my spec for the function I'm supposed to write
12:00:20 PM - Vlad: mmmm
12:00:26 PM - Simplex: like does it take a (nsIFile* filename)
12:00:38 PM - Vlad: well, you're writing more than a function :)
12:00:41 PM - Vlad: a couple of things you need to do
12:00:53 PM - Vlad: 1) need to look at the data that's stored in signons.txt,
and create a SQL schema for storing that data
12:01:14 PM - Vlad: 2) need to create a function that creates those SQL tables
in the profile storage database
12:01:32 PM - Vlad: 3) need to create something that will read the current
signons.txt, if it exists, and insert that data into the database
12:01:55 PM - Vlad: 4) need to modify the passwordmgr to do queries against the
database instead of using its own internal stuffs
12:02:49 PM - Simplex: since 1, 2, and 3 only get called once, to I throw them
in a new file?
12:03:23 PM - Vlad: probably no need, they should be small
12:03:30 PM - Simplex: ok, cool
12:03:33 PM - Simplex: that really helps
12:03:35 PM - Vlad: #2 in particular needs to check for whether the tables
already exist, and if not, only then create them
12:03:53 PM - Vlad: the schema should be pretty simple for this
12:03:56 PM - Vlad: so that shouldn't be too bad
12:03:57 PM - Simplex: I'm sure I'll have to come back to you to figure out how
to talk to the db for this
12:04:06 PM - Vlad: np
12:04:10 PM - Vlad: there are some good examples in
12:04:37 PM - Vlad:
http://lxr.mozilla.org/seamonkey/source/calendar/providers/storage/calStorageCalendar.js
12:04:42 PM - Simplex: I'm filing a bug for this, would it co under the "core"
component?
12:04:54 PM - Vlad: in javascript, but it translates to the same function calls
in C++
12:05:03 PM - Vlad: specifically look at "initDB"
12:05:09 PM - Vlad: it would go under... one sec
12:06:51 PM - Vlad: Toolkit product, Password Manager component (which doesn't
exist yet, but it will shortly)
12:07:15 PM - Simplex: thanks, vlad
12:07:22 PM - Simplex: do I need editbugs to edit my own bugs?
12:08:46 PM - Vlad: nope, you should have full access to your own bugs
12:08:48 PM - Vlad: if not, let me know
12:09:02 PM - Vlad: if you want to file a tracking bug for this for now though,
use Toolkit -> Storage
12:09:07 PM - Vlad: we can move it to the other component once it shows up
12:09:12 PM - Simplex: will do
12:12:09 PM - Simplex: this will all be working off of the firefox trunk, right?
12:12:13 PM - Simplex: I can use that codebase?
12:12:53 PM - Vlad: yup
12:12:58 PM - Simplex: k thanks
12:12:58 PM - Vlad: do you have firefox built from source?
12:13:09 PM - Vlad: I'd get a build done first before going all crazy on it
12:13:11 PM - Simplex: I don't right now, but I've done it before
12:13:15 PM - Vlad: also add --enable-storage to your mozconfig
12:13:20 PM - Simplex: ok
12:13:23 PM - Vlad: otherwise you won't end up with the db bits :)
12:13:32 PM - Simplex: that'd be good
Status: NEW → ASSIGNED
Summary: convert signons.txt to MozStorage → convert signons.txt to Unified Storage
"1) need to look at the data that's stored in signons.txt,
and create a SQL schema for storing that data"

I'd propose this way of storing data in SQLite:

CREATE TABLE signon_fields (url blob, field_id blob, field_value blob,
is_password boolean);

CREATE TABLE signon_blacklist (url blob);

SQLite is typeless (http://www.sqlite.org/datatypes.html) so though boolean
doesn't work under MySQL, it sounds like it should work in SQLite.

I think that having two tables, while unfortunate, is the best representation of
the way that the data is understood.
(In reply to comment #2)
> "1) need to look at the data that's stored in signons.txt,
> and create a SQL schema for storing that data"
> 
> I'd propose this way of storing data in SQLite:
> 
> CREATE TABLE signon_fields (url blob, field_id blob, field_value blob,
> is_password boolean);
> 
> CREATE TABLE signon_blacklist (url blob);

Two tables is fine; however, url and field_id should just be varchar (since
they're simple readable text, unicode safe).  The other two are fine as blobs,
depending on whether you want to base-64 encode stuff before storing or not.
I'm worried about a couple things in transition to SQL here:

1) If there are multiple values for a certain field, the old way would take the
most recently added.  When running an sql query, I'm not sure which one comes up
first, or even a way to pick the most recently added to mimic the old behavior.

2) The second feels like a bug:
1. Go to http://hope.homedns.org/capstone/
2. Go to the first signon test
3. Sign on and save it
4. Go the second signon page
5. Sign on as something different and select "Never for This Site"
6. Tools -> Options -> Privacy -> Saved Passwords -> View Saved Passwords
7. Note that your saved password is still there, as is the fact that your domain
is on the tab that says "... will never save logon information for the following
sites".

I can immitate the 2nd, but should I just fix it while doing the transition?
(In reply to comment #2)
> CREATE TABLE signon_blacklist (url blob);
IF you are converting things, you might as well convert this to
nsIPermissionManager. No need for more host-to-permission lists (at least in my
opinion)
vlad, since I know nothing of overall structure, and you do, I need you to
answer mvl's questions in comment 7, and maybe look at mine in comment 6.  Thanks!
After comment 5 and further looking at the code, here's a better way to store
the tables, that goes with Brian Ryner's original naming conventions:

CREATE TABLE signon_fields (realm text, field_name blob, field_value blob,
is_password boolean);

CREATE TABLE signon_blacklist (realm text);
(In reply to comment #6)
> I'm worried about a couple things in transition to SQL here:
> 
> 1) If there are multiple values for a certain field, the old way would take the
> most recently added.  When running an sql query, I'm not sure which one comes up
> first, or even a way to pick the most recently added to mimic the old behavior.

This is possible; when you do a select, you can do "SELECT oid,* FROM ... ORDER
BY oid DESC" and you'll get them ordered by the time they were entered (or
better if there's an actual date field somewhere you can sort by).

> 2) The second feels like a bug:

It probably is a bug; bryner would probably be the one to say for sure though.

I'm not sure what mvl meant by his comments; mvl, are you thinking to just use
the permissions manager service to keep track of the blacklist, as opposed to
having the passwordmgr store them itself?
(In reply to comment #10)
> This is possible; when you do a select, you can do "SELECT oid,* FROM ... ORDER
> BY oid DESC" and you'll get them ordered by the time they were entered (or
> better if there's an actual date field somewhere you can sort by).

I thought about adding a date column, but the text file is LIFO (stack), but
when I parse the text file and put it into the db, I'd add them FIFO (queue) and
then the order would be reversed.  I like your way MUCH better, not to mention
the added complexity of adding a date (which could be wrong on the client's side
leading to more bugs).
> mvl, are you thinking to just use
> the permissions manager service to keep track of the blacklist, as opposed to
> having the passwordmgr store them itself?
Yeah, that's my idea. No need to have another list of hosts and matching code,
when nsIPermissionManager can do it just fine.
Attached patch Initial Patch v1 (obsolete) — Splinter Review
"12:01:14 PM - Vlad: 2) need to create a function that creates those SQL tables

in the profile storage database"

This does just that.  It's not enclosed in it's own function -- it goes
straight into nsPasswordManager::Init() so my code runs when the password
manager gets created.  It checks to see if a table is already there based on
the fact that a table has != 0 columns.  

Notes:
I'm pretty new at this, so feel free to smack me around if I've done something
wrong or just plain silly.

I used NS_ENSURE_SUCCESS and NS_ASSERTION in the style of the code around me. 
I don't really understand what will happen (I mean, I can guess what will
happen) because I didn't run into a time where any of these asserts came up.

I had to add "storage" in the Makefile of a couple of things that include
nsPasswordManager.h, since that now has a mozStorage include.

My text editor gets rid of trailing spaces, so a couple of those are gone.  If
that annoys somebody, I can change it (though it goes along with the mozilla
style guidelines).

It compiles. =)
Attached patch Initial Patch v1.1 (obsolete) — Splinter Review
Mostly the same as the last patch, except that it puts things in functions,
since I'll just be doing that later.  It also changes the logic just a bit.

If either table isn't there, it will go ahead and create both.	If there is an
error doing ExecuteSimpleSQL during the create, it will return the first
occurance of the problem, but will still have attempted to create both tables.
Attachment #179507 - Attachment is obsolete: true
Attached patch Initial Patch v1.2 (obsolete) — Splinter Review
Oops, wrong patch.  This is the correct one.  Sorry!
Attachment #179533 - Attachment is obsolete: true
Attached patch Initial Patch v1.3 (obsolete) — Splinter Review
I cleaned up a lot, used mDB->CreateTable to create tables, instead of doing it
via ExecuteSimpleSQL, and deleted file after I read it.  Hopefully this is a
stepping stone into getting the last big step done here, and that is making all
of the logic change because of a data structure change.  Here we go...
Attachment #179534 - Attachment is obsolete: true
Attached file corresponding sql tables (obsolete) —
"You learn something new everyday."

Today I learned that when you want to delete a signon entry, you only have to
specify the userField, and it should delete the entire signon, userField,
userValue, passField, and passValue.  To facilitate that, I had to change the
schema a bit.  Now, what the old logic would call an "entry" is now a single
row in the db.	Delete one, you delete them all.
Attachment #178844 - Attachment is obsolete: true
Things I need to remember to help develop, but didn't have a good place to write
down:

- Setting NSPR_LOG_MODULES=mozStorage:5 in bash gives me what the SQLite backend
is doing.

- For entire tables: "SELECT * FROM tablename;" and "DELETE FROM tablename;"

- fprintf(stderr, "%s", PromiseFlatCString(some_nsACString).get());
- NS_ConvertUCS2toUTF8 buffer(some_nsAString); fprintf(stderr, "%s", buffer.get());

- runtime crashes (for this bug) are almost always caused by faulty SQL statements.
Attached patch Low Risk Patch v1 (obsolete) — Splinter Review
I had envisioned replacing the hash tables mSignonTable and mRejectTable with
the sql database as the main data structure, but that has a lot of risk
associated with it.  This patch is low risk, because it just uses the SQLite
database to store the signon data between browser sessions, instead of the
signons.txt flat file.

This is my first non-trivial patch, so I'm asking the reviewer to thoroughly go
through it and check for things like bad style and memory leaks.  I added a
couple of procedures, and I just need verification that they all went in their
right spots, and returned the right types of values.

I tested this patch by starting Firefox 1.0 (Ubuntu 1.0.2) and then running
around getting both rejected urls and saved passwords.	Then I ran my build on
top of my old profile, checking to see if the data migrated over properly. 
Then I restarted Firefox and again checked to see if holding data in the
database worked.  I then continued to run around and add/remove signon data and
made sure that stuck.

LoadPasswords() tries to get as much data as it can, even given errors.  Any
data not taken out of the db after LoadPasswords() is called will most likely
be lost.  So if I get an error, I remember that I got an error, but I try to
breeze past it and grab more data without crashing.  I used NS_ASSERT and
NS_ENSURE_SUCCESS in Init() as best I could to give clues as to what, if
anything, went wrong.
So you decided to not use nsIPermissionManager (at least for now)?
For now, yes.  I want to create a low-risk patch that doesn't change any
behavior.  When I remove the hash tables and start using the db as the primary
data structure, I will look into consolidating the data.  I'm going to file
another bug about doing all of this (and possibly your suggestion of using
nsIPermissionManager) after this one gets done.
Attachment #181167 - Flags: second-review?(vladimir)
Attachment #181167 - Flags: first-review?(bryner)
bsmedberg said not to delete signons.txt so that we can roll back to old
versions without huge dataloss (anything you add while on the storage db will
not roll back to the file, however).  So I'll add that to the list of things I
change once I get a full review.
Fixing bitrot from bug 274784 (bfcache).
Attachment #181167 - Attachment is obsolete: true
Attachment #182744 - Flags: second-review?(vladimir)
Attachment #182744 - Flags: first-review?(bryner)
Attachment #181167 - Flags: second-review?(vladimir)
Attachment #181167 - Flags: first-review?(bryner)
Attachment #182744 - Flags: second-review?(vladimir)
Attachment #182744 - Flags: first-review?(bryner)
This patch is probably obsolete, since the password manager rewrite (bug 374723) has landed.
Assignee: mikegoodspeed → nobody
Status: ASSIGNED → NEW
Component: Storage → Password Manager
Product: Toolkit → Firefox
QA Contact: nobody → password.manager
Version: unspecified → Trunk
Are you still interested in doing this, Mike? It's probably easier now that the PW mgr storage component has been abstracted out and given it's own interface. I know that Justin Dolske has been considering this as part of his password manager rewrite.
Assignee: nobody → mikegoodspeed
Thanks for asking, Gavin.  I'm not currently interested in keeping up with bug for a couple of reasons:

1) It's probably not the best solution.
2) I still have a bad taste in my mouth from writing the patch, never getting a review, updating due to bitrot, and still never getting a review.  Maybe someone closer to the project can actually get something done.
3) The ubiquitous "I don't have enough time anymore" reason.

I'm still on the lookout for something I can reasonably do to help the project.  I may or may not find something later.  Who knows? :)
Fair enough. I'm sorry about point 2, getting review as a new contributor to the codebase is often more difficult than it should be.
Assignee: mikegoodspeed → nobody
Summary: convert signons.txt to Unified Storage → convert signons2.txt to mozstorage
Severity: normal → enhancement
(In reply to comment #27)
> Fair enough. I'm sorry about point 2, getting review as a new contributor to
> the codebase is often more difficult than it should be.
> 

Hi Gavin, in case this feature is needed, I have already done it. Here I will explain it briefly. I am not familiar with the cvs tree, etc. so I will speak of files as they are found when installed (Firefox 3.0b2 on Win XP in my case). It goes as follows:

New file:
<Install Directory>\components\storage-Sqlite.js

Changed files:
1. <Install Directory>\greprefs\all.js has the following new preference
pref("signon.SignonFileName4",              "signons.sqlite");

2. line 90 of <Install Directory>\components\nsLoginManager.js changed to
this.__storage = Cc["@mozilla.org/login-manager/storage/sqlite;1"]
                                .createInstance(Ci.nsILoginManagerStorage);

this class (@mozilla.org/login-manager/storage/sqlite;1) is implemented in storage-Sqlite.js

As of now, the code doesn't take much advanrage of db. Like with the legacy files, it drops the tables, creates them anew and inserts data into them each time _writeFile is called. This can be easily changed if needed for performance (which would be adversely affected only if someone stores thousands of logins). 
Missed the attachment in my comment# 28. This file imports the legacy signons text files to sqlite db file (if not already done) and uses the sqlite db file for all future operations with signons.
(In reply to comment #28)
> Hi Gavin, in case this feature is needed, I have already done it.

Great! I haven't taken a close look at your implementation yet - it would help if you could attach it in patch format. There is documentation available for generating a patch and attaching it, you could start with http://developer.mozilla.org/en/docs/Mozilla_Source_Code_Via_CVS to get a source tree, and then http://developer.mozilla.org/en/docs/Creating_a_patch for creating a patch specifically.

Fixing a bug like this one probably isn't the easiest way to start contributing to Firefox, but I'd certainly be glad to help you go through the process. One of the problems to consider would be migration of data - it doesn't look like the work you've attached takes into consideration migration from the old format to the new - perhaps we could review and land your new implementation so that it lives side-by-side with the old one and figure the migration part out separately.
(In reply to comment #30)

Yes, migration from the old format to the new has been taken care of in the attached work. 

Let me try to create a patch following the links in the above comment.
(In reply to comment #31)
> Yes, migration from the old format to the new has been taken care of in the
> attached work. 

Ah, excellent!

> Let me try to create a patch following the links in the above comment.

Once you have the patch ready you should ask for review according to the instructions at http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree - dolske@mozilla.com would be a good choice for a first reviewer since he wrote the original legacy implementation.
Assignee: nobody → mrinal.kant
Attachment #180288 - Attachment is obsolete: true
Attachment #180305 - Attachment is obsolete: true
Attachment #182744 - Attachment is obsolete: true
Comment on attachment 294684 [details]
this file will replace storage-Legacy.js to use sqlite db for signons

changed the description of the attachment #294684 [details].
Attachment #294684 - Attachment description: this file will replace sqlite-Legacy.js to use sqlite db for signons → this file will replace storage-Legacy.js to use sqlite db for signons
I have to admit that I haven't looked at the patch but I don't want this to be the third conversion to sqlite for which I need to file a followup bug to provide an import API for profile migration to be able to use ;-)
I guess I'm going to be a party pooper here. :-/ I'm mainly concerned that it's late in the 1.9 cycle... Although we should switch to mozstorage at some point, right now doing so doesn't add much value or fix a problem -- but does add risk of introducing new problems / regressions, and diverts focus from stuff that needs fixed to ship.

I think the patch is a good start, though, so I'm not discouraging working on it! It would be great to land a switch to mozstorage as soon as Firefox 3 branches from trunk (or whatever the equivalent is with the move to Hg and all...).

Some general comments from a quick skim:

* Just replacing readFile/writeFile sorta seems like a nice way to transition, but I think it would be better to just go ahead and eliminate those functions, and have the various nsILoginManagerStorage interfaces do direct queries. [IE, get rid of the |_logins| and |_disabledHosts| data structures.]

* Do this all in a new component -- storage-SQL.js or somesuch -- and have that component use storage-Legacy.js to handle the initial data import. For example, have an init() that looks something like:

init() {
  if (!exists("signons.sqlite")) {
     newDB = createNewDB();
     legacy = CC["...storage/legacy;"].getService();
     logins = legacy.getAllLogins();
     for each (var login in logins) {
       this.addLogin(login)
     }
  }
}
(In reply to comment #35)
> I guess I'm going to be a party pooper here. :-/ I'm mainly concerned that it's
> late in the 1.9 cycle... Although we should switch to mozstorage at some point,
> right now doing so doesn't add much value or fix a problem -- but does add risk
> of introducing new problems / regressions, and diverts focus from stuff that
> needs fixed to ship.

I agree - I didn't mean to imply that we should land this for Firefox 3. Landing it after branching/switching/whatever as you suggest is the way to go, I think.
Attached file storage-Sqlite.js (obsolete) —
I have attached a new js file which addresses points brought out by Justin.
Justin had some general comments on the attachment #294684 [details]:

Justin's comment =  Just replacing readFile/writeFile sorta seems like a nice way to transition, but I think it would be better to just go ahead and eliminate those functions, and have the various nsILoginManagerStorage interfaces do direct queries. [IE, get rid of the |_logins| and |_disabledHosts| data structures.]

response: yes, the functions have been changed accordingly.

Justin's comment =  Do this all in a new component -- storage-SQL.js or some such -- and have that component use storage-Legacy.js to handle the initial data import. 

response: that would be a neat solution, but wouldn't it mean retaining two classes, one of them just for import. I have, as of now, included the required functions (for importing from legacy files) in the storage-Sqlite.js itself.
----------------

PS: still trying to make the "make" work on Windows. Never done this before on Windows OS. So, the patch will have to wait until I have access to Linux machine.
Attachment #294684 - Attachment is obsolete: true
(In reply to comment #37)

> response: that would be a neat solution, but wouldn't it mean retaining two
> classes, one of them just for import. I have, as of now, included the required
> functions (for importing from legacy files) in the storage-Sqlite.js itself.

There would be two components, yes. But it would be modular, and keeps the SQL component simpler. Other storage components will probably want to use the legacy code for import too (OS X Keychain, I'm looking at you).
Changed the class to use storage-Legacy.js to import data from legacy signon files. Justin's comment #38 convinced me why it should be done this way. That it would be a neater solution was already clear.

Also, a question: what is the function initwithfile for? what exactly does it do and who calls it?
Attachment #294879 - Attachment is obsolete: true
(In reply to comment #39)
> Also, a question: what is the function initwithfile for? what exactly does it
> do and who calls it?
> 
I meant the function initWithFile in storage-Legacy.js
Perhaps, Justin could best answer that.
for the function void initWithFile(in nsIFile aInputFile, in nsIFile aOutputFile), the following description is offered in nsILoginManagerStorage.idl


* Initialize the component, but override the default filename
* locations. This is primarily used to the unit tests and profile
* migration.
* @param aInputFile
*        File to read for stored logins.
* @param aOutputFile
*        If non-null, file to output logins to.

     void initWithFile(in nsIFile aInputFile, in nsIFile aOutputFile); 
--------------------------------------------
In the context of signons.sqlite/storage-Sqlite.js, does the following assumption appear reasonable?

1. aOutputFile == null implies that aInputFile is an sqlite file and no migration is necessary. However, if aInputFile does not exist should we (a) attempt to migrate the data from legacy files or (b) just create the sqlite file and attempt no migration at all. 
2. aOutputFile != null implies that aInputFile is a legacy file and aOutputFile is an sqlite file. Import data from aInputFile and write (overwrite, if aOutputFile already exists) to aOutputFile.

Comments please.
Based on these comments, I will implement the initWithFile function.
Hey,
I've been assigned to this bug as a part of my school project. See here:
http://zenit.senecac.on.ca/wiki/index.php/Convert_password_storage_to_a_SQLite_database
It seems someone here is already working on it. 

So what is the current status of this? Where could i help?

I'll wait for your reply for a week or so (until next Friday let's say) before trying to dig into it myself.

(My nick on IRC is radoye)
(In reply to comment #43)

> So what is the current status of this? Where could i help?

Mrinal Kant would be the one to answer that.

I think the basic design is on the right track, but an in-depth review isn't likely to happen until we after Firefox 3 ships.

Hmm, while I'm here, if someone was to update the mozstorage stuff here to work with the patch in bug 386533, that would (1) be awesome for testing this as an add-on, and (2) be awesome to help validate the approach of 386533.
(In reply to comment #43)
> Hey,
> I've been assigned to this bug as a part of my school project. See here:
> http://zenit.senecac.on.ca/wiki/index.php/Convert_password_storage_to_a_SQLite_database
> It seems someone here is already working on it. 
> 
> So what is the current status of this? Where could i help?
> 

Please go through the attachment storage-Sqlite.js (https://bugzilla.mozilla.org/attachment.cgi?id=294907).

As far as I can tell, all the functionality is correctly implemented except the function initWithFile, about the behaviour of which I am not clear (see comment #42).

The other changes required for using storage-Sqlite.js are described in comment #28.

The reason that I stopped working on this was that I could not create a patch on my computer (WinXP) owing to problem with make while getting source using CVS. Must be something silly on my part, but what was a cakewalk on Linux turned out to be so difficult on WinXP and there is no possibility of me getting access to Linux in the near future. So, I think this bug could be assigned to someone else.
OK then, i presume this means I'm free to take over. 

I hope you'll still be around if i need to ask you a few questions, i feel I'm going to need to do that!

Thanks!
Attached a patch of modules/libpref/src/init/all.js

A new preference signon.SignonFileName4 with the value of signons.sqlite is needed in all.js

Thanks to Nickolay Ponomarev for guiding me on how to create patches, build, etc. on Win XP.
The only change is to use contract id @mozilla.org/login-manager/storage/sqlite;1 instead of that for legacy for storage.
(In reply to comment #46)
> OK then, i presume this means I'm free to take over. 
> 

The status of this bug has changed since I last replied to your comment.
I have created the patches of the two files that require the changes as described in comment #28. As for storage-Sqlite.js (attachment id=294907), this file sits in the toolkit/components/passwordmgr/src directory.

The only area that, in my opinion, requires some work is the initWithFile function in storage-Sqlite.js

> I hope you'll still be around if i need to ask you a few questions, i feel I'm
> going to need to do that!
> 
Sure, I am around. I will be happy to answer any questions if I have the answers.
(In reply to comment #49)
Thanks Mrinal, sorry for not replying sooner, i was tied up in an unrelated project, but i did read your comments and appreciate them. I've been reading your code - i'm still not familiar enough with the Mozilla source base and need to get a better grip on what fits exactly where, and also have only a limited JavaScript knowledge, being a C/C++ guy primarily.

Anyway, i think i'll be ready soon to be able to contribute to this project in a much more meaningful way.
FYI Mrinal - the second patch you made, the one for nsLoginManager.js does not work since someone modified that portion of the file. I'll see to make it work with these modifications.
(In reply to comment #51)
> FYI Mrinal - the second patch you made, the one for nsLoginManager.js does not
> work since someone modified that portion of the file. I'll see to make it work
> with these modifications.
> 

Yes, I am aware. The use of getCategoryEntry("login-manager-storage", "nsILoginManagerStorage")while using "@mozilla.org/login-manager/storage/legacy;1" as the default means that there should be no need to do anything at all to nsLoginManager.js. 

Instead, we should add corresponding category entry so that the getCategoryEntry function would return "@mozilla.org/login-manager/storage/sqlite;1". 
Comment on attachment 301701 [details] [diff] [review]
patch of toolkit\components\passwordmgr\src\nsLoginManager.js

made this attachment obsolete in view of comment #52. sorry for the delay; and thanks for the reminder in comment #51.
Attachment #301701 - Attachment is obsolete: true
(In reply to comment #52)
> The use of getCategoryEntry("login-manager-storage",
> "nsILoginManagerStorage")while using
> "@mozilla.org/login-manager/storage/legacy;1" as the default means that there
> should be no need to do anything at all to nsLoginManager.js. 
> 
> Instead, we should add corresponding category entry so that the
> getCategoryEntry function would return
> "@mozilla.org/login-manager/storage/sqlite;1". 

I'm in way too deep over my head here so forgive me if i'm talking nonsense now, but looking at this it seems to me what needs to be done to fix this is to test for the default category entry using the getCategoryEntry method and if it's pointing to the 'legacy' replace it with the 'sqlite' one using the addCategoryEntry method (both being in nsCategoryManager.cpp). Sounds plain and simple. But where do we add this check? This needs to go in at the point where the whole 'remember login' process is being initiated, when the user decides to save the login info and clicks the appropriate button. Am i correct?
(In reply to comment #54)

> but looking at this it seems to me what needs to be done to fix this is to
> test for the default category entry using the getCategoryEntry method and if
> it's pointing to the 'legacy' replace it with the 'sqlite'

Normally there is nothing in the category... This is the hook for alternative implementations to override Firefox's default. The storage-Sqlite.js code will presumably be the default in FF at some point -- so a change like this would be expected.

We might need some kind of import/export nexus for the general problem of switching from Implementation X to Implementation Y (while keeping existing logins), but that problem doesn't need to be solved here.
(In reply to comment #55)

> We might need some kind of import/export nexus for the general problem of
> switching from Implementation X to Implementation Y (while keeping existing
> logins), but that problem doesn't need to be solved here.

Yeah well unfortunately my priorities are a bit different than those of the Mozilla world at large, since i got assigned to this as a part of a school project and i need to come up with something that is actually functional by the end of the semester (April - it's getting darn close!). I am aware that this bug is on the backburner and not being considered for the FF3 release, but i still need to make it work within the current Minefield to pass the course.

So maybe figuring out this multiple implementation issue could be a good start for me - though it seems like a really huge task :/

I had an opportunity to chat with Mike Shaver the other day and he suggested that maybe at this point (due to the approaching ff3 release) the best solution would be to package this as an extension rather than a patch. SQLite password storage is going to be a default in the near future (post-ff3) but having it as an extension now would allow ff3 users to use this too if they wish and it could be included as such in any future release. Going down this path would also allow me to actually start making some progress and meeting my deadlines.

Any thoughts on this?
(In reply to comment #57)
> I had an opportunity to chat with Mike Shaver the other day and he suggested
> that maybe at this point (due to the approaching ff3 release) the best solution
> would be to package this as an extension rather than a patch. SQLite password
> storage is going to be a default in the near future (post-ff3) but having it as
> an extension now would allow ff3 users to use this too if they wish and it
> could be included as such in any future release. Going down this path would
> also allow me to actually start making some progress and meeting my deadlines.
> 
> Any thoughts on this?
> 

Yes, I have already experimented with an extension. And I have faced the following issue:
using the extension I addCategoryEntry with persist = true (4th argument of that function). But when I restart firefox, the added category entry is not found. Maybe, a problem with my understanding.
The following is an excerpt from that extension:
------------------------------------------------------
useSqliteStorage: function()
{
    var cID_legacy = "@mozilla.org/login-manager/storage/legacy;1";
    var cID_sqlite = "@mozilla.org/login-manager/storage/sqlite;1";

    var category = "login-manager-storage";
    var entry = "nsILoginManagerStorage";

    var newContractID = cID_legacy;
    var bUse = sm_getPreferenceValue("useSQLite", false);   
    if (bUse)
      newContractID = cID_sqlite;

    var oldContractID = null;
    try {
      var catMan = Cc["@mozilla.org/categorymanager;1"].
                   getService(Ci.nsICategoryManager);
      oldContractID = catMan.addCategoryEntry(category, entry, newContractID,
                        true, true);
      if (this.m_currentCID == "none")
      {
        this.m_currentCID = oldContractID;
        if (this.m_currentCID == null)
          this.m_currentCID = cID_legacy;
      }

      newContractID = catMan.getCategoryEntry(category, entry);
    } catch (e) {
      alert("Exception occured. Operation failed!");
      return false;
    }

    if (this.m_bAlert && oldContractID != newContractID) 
    {
      alert("You have decided to use a different class to store your login details:" +
            "\nCurrent contract ID:  " + this.m_currentCID +
            "\nNew contract ID:  " + newContractID +
            "\n\nPlease restart firefox for the changes to take effect.");
    }
    return true;
}
----------------------------------------
Even if that worked, one would have to restart firefox because nsLoginManager.js chooses the relevant contractID just once.

I was also wondering whether category manager has some advantages over using just a preference to store the contract id. Wondering because when use of category manager was being discussed (which I knew nothing about then) I was thinking of using preference instead. Just a thought, this last para!
(In reply to comment #58)

> using the extension I addCategoryEntry with persist = true (4th argument of
> that function). But when I restart firefox, the added category entry is not
> found. Maybe, a problem with my understanding.

Huh, that might be a bug, but I'm not familiar with the category manager internals. File a bug?

In any case, you can work around by adding the category entry when the component is registered. That's seems like a better way, anyway, because if the extension should be uninstalled, you wouldn't want the category entry sticking around. [And perhaps nsLoginManager.js should fallback to the default if a category exists but fails to init? Hmm.]
So is there still some action happening on this bug, or has it fallen by the wayside?

I would be interested in taking up the reins here. I've been making a few contributions to the password manager recently, so have some familiarity with this. It seems to make sense that we switch over to SQLite, and now that Fx3 has launched, we might be able to get this into 3.1
Attached patch SQLite Patch v0.1 (obsolete) — Splinter Review
This takes the work established above, rewritten to be "better", and works to replace the legacy system. It will import your old signons3.txt.

Not looking for review yet because I don't think this is up to par yet, but would like some feedback.

BTW, this uses the statement wrapper for SQLite so that parameter replacement is much easier. It can be done the way proposed in the original patch, but this provides a layer of defense against bad strings (or so I'm lead to believe).

Also, maybe we can get this wanted-firefox3.1?
Attachment #294907 - Attachment is obsolete: true
Attachment #301700 - Attachment is obsolete: true
Flags: wanted-firefox3.1?
(In reply to comment #61)
> Created an attachment (id=328522) [details]
> SQLite Patch v0.1
> 
Looks great! I think this bug should be assigned to poshannessy@mozilla.com
Not sure whether I can do that but I will try the reassign option below.
Assignee: mrinal.kant → poshannessy
Comment on attachment 328522 [details] [diff] [review]
SQLite Patch v0.1

drive-by review

general comments:
s/var/let/ for perf!
you may want to compile statements (or have lazy getters) that can get reused for perf

>diff --git a/toolkit/components/passwordmgr/src/Makefile.in b/toolkit/components/passwordmgr/src/Makefile.in
>+			storage-SQLite.js \
storage-mozStorage?

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManager.js b/toolkit/components/passwordmgr/src/nsLoginManager.js
>-            var contractID = "@mozilla.org/login-manager/storage/legacy;1";
>+            var contractID = "@mozilla.org/login-manager/storage/sqlite;1";
.../storage/mozStorage;1?


>diff --git a/toolkit/components/passwordmgr/src/storage-SQLite.js b/toolkit/components/passwordmgr/src/storage-SQLite.js
>+        // Create the tables
>+        if (!this._signonsFile.exists()) {
>+            this._createTables();
>+        }
This doesn't take into account the fact that you might want to change the db schema somewhere down the line.  All our other database consumers have some kind of framework for this.

>+        var query = "INSERT INTO " + this._loginsTableName +
>+                     " (hostname, httpRealm, formSubmitURL, usernameField," +
>+                     "  passwordField, encryptedUsername," +
>+                     "  encryptedPassword) VALUES" +
>+                     " (:hostname, :httpRealm, :formSubmitURL," +
>+                     "  :usernameField, :passwordField, :encryptedUsername," +
>+                     "  :encryptedPassword)";
Just start the query on the next line to help with line wrapping?
Also, don't see the point of this._loginsTableName at all.

>+         var params = {hostname: login.hostname,
>+                       httpRealm: login.httpRealm,
>+                       formSubmitURL: login.formSubmitURL,
>+                       usernameField: login.usernameField,
>+                       passwordField: login.passwordField,
>+                       encryptedUsername: encUsername,
>+                       encryptedPassword: encPassword};
nit:
let params = {
  param: value,
};
(keep comma for every line, bracing style)

>+
>+             var stmt = this._createStatement(query, params);
should be in the try block (with later comments about createStatement addressed)

>+        try {
>+ 
>+            stmt.execute();
>+            stmt.statement.finalize();
don't want this if you use pre-prepared statements

>+        } catch (e) {
>+            throw "Couldn't write to database, login not added.";
should reset here

>+        // do checks for null and empty strings, adjust params
>+        if (login.formSubmitURL == null) {
>+            query += " AND formSubmitURL isnull";
>+        } else if (login.formSubmitURL != '') {
>+            query += " AND formSubmitURL = :formSubmitURL";
>+            params["formSubmitURL"] = login.formSubmitURL;
>+        }
>+        if (login.httpRealm == null) {
>+            query += " AND httpRealm isnull";
>+        } else if (login.httpRealm != '') {
>+            query += " AND httpRealm = :httpRealm";
>+            params["httpRealm"] = login.httpRealm;
>+        }
Please please please do not dynamically build queries like this.  I'm also not really sure what is trying to be accomplished here with all these special checks.

>+        // Disabled hosts kept, as one presumably doesn't want to erase those.
>+        // Or should we follow old behavior here?
>+        // FIXME update to use try-catch?
>+        var query = "DELETE FROM " + this._loginsTableName;
>+        var stmt = this._createStatement(query, {});
>+        stmt.execute();
>+        stmt.statement.finalize();
FIXME is a yes ;)
You need to reset the statement at least so a potential database lock can be undone.

>+    _createStatement : function (query, params) {
javadoc comment please

>+        // FIXME I think the try-catch blocks can be trashed
>+        //       if we assume we'll get OK params
just make the caller use the try catch, since they should be anyway

>+        var wrapper = new Cco("@mozilla.org/storage/statement-wrapper;1",
>+                              Ci.mozIStorageStatementWrapper, "initialize");
>+
>+        this.log("Creating query from \"" + query + "\" and params " +
>+                 params.toSource());
>+        var _dbConnection = this._dbConnection;
>+        var wrapper_func = function createStatement (query) {
>+            try {
>+                return new wrapper(_dbConnection.createStatement(query));
You also might just want to do something like this:
http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_storage_statement_wrapper.js#46

>+        for (var i in params) {
>+            try {
>+                stmt.params[i] = params[i];
>+            } catch (e) {
>+                 this.log("Error: " + e.name + " : " + e.message);
>+            }
ditch try/catch, as well as all bracing

I'd also make this function not require params (you use it that way at least once).  This just needs a simple if check.
Comment on attachment 328522 [details] [diff] [review]
SQLite Patch v0.1

Some quick comments. I didn't look at the actual SQL stuff closely, you might ask sdwilsh for comments on that.

>+pref("signon.SignonFileName4",              "signons.sqlite");

Let's not pref this. Does the DB keep a schema version somewhere, so that future format changes can be done?

>+    /*
>+     * A list of prefs that have been used to specify the filename for storing
>+     * logins. (We've used a number over time due to compatibility issues.)
>+     * This list is also used by _removeOldSignonsFile() to clean up old files.
>+     */
>+    _filenamePrefs : ["SignonFileName4", "SignonFileName3", "SignonFileName2", "SignonFileName"],

*sadface* Guess we need to keep this for removing the old files, though.

>+            // this initialization needed for getAllXxxx functions to work
>+            var count = [];
>+            // Import all logins and disabled hosts
>+            var logins = legacy.getAllLogins(count);

You can just pass in a empty object here: .getAllLogins({}). XPCOM is stupid about the array size outparam, which isn't needed for JS callers. (bug 380597)

One tricky case to test here is if getting the logins triggers a master password prompt, but  the user refuses to enter it.

>+    addLogin : function (login) {
>+        this.log("Adding login: " + login.toSource());

This is ok for development, but for the final version we want to be sure not to log passwords anywhere.

>+        // Throws if there are bogus values.
>+        this._checkLoginValues(login);

Hmm. In theory the DB should be able to store arbitrary data, but it might be wise to do some bogus-data filtering anyway.

>+        // We rely on using login.wrappedJSObject. addLogin is the
>+        // only entry point where we might get a nsLoginInfo object
>+        // that wasn't created by us (and so might not be a JS
>+        // implementation being wrapped)
>+        if (!login.wrappedJSObject) {
>+            var clone = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+                        createInstance(Ci.nsILoginInfo);
>+            clone.init(login.hostname, login.formSubmitURL, login.httpRealm,
>+                       login.username,      login.password,
>+                       login.usernameField, login.passwordField);
>+            login = clone;
>+        }

I suspect this stuff won't be needed any more. The legacy implementation needed it because everything was stored in-memory, and so logins needed to remember their encrypted values for when we dumped to signons3.txt. Now there's no in-memory storage (we work with the DB directly), so this hack can be avoided.


>+    /*
>+     * removeLogin
>+     *
>+     */
>+    removeLogin : function (login) {
...
>+
>+        var query = "DELETE FROM " + this._loginsTableName +
>+                    " WHERE hostname = :hostname" +
>+                    " AND usernameField = :usernameField" +
>+                    " AND passwordField = :passwordField" +
>+                    " AND encryptedUsername = :encryptedUsername" +
>+                    " AND encryptedPassword = :encryptedPassword";

This won't work. The encrypted strings have random salts in them, so no two encrypted values will be the same (even when the plaintext values are). This either needs to do what the old code did (get possible logins, decrypt them, then compare with the provided login), or we could looks at adding a GUID to the login for unique identification. That might come in handy other places here, and for external callers like Weave.


>+    modifyLogin : function (oldLogin, newLogin) {
>+        // Throws if there are bogus values.
>+        this._checkLoginValues(newLogin);
>+
>+        // TODO: Create a single update statement, or wrap in transaction
>+        this.removeLogin(oldLogin);
>+        this.addLogin(newLogin);
>+    },

Yeah, this would be nice to finally fix.


>+    _queryLoginInfo : function (whereClause, params) {
...
>+        var nsLoginInfo = new Cco("@mozilla.org/login-manager/loginInfo;1",
>+                                  Ci.nsILoginInfo);

"new Cco"? Ugh. Either createInstance, or use a constructor. (Look for "Components.Constructor" in the legacy code).
Attached patch SQLite Patch v0.2 (obsolete) — Splinter Review
This takes most of what the last 2 comments said into account.  I'm not doing any lazy getters for the queries. Also, "modify" is still a bit hacky, but it seemed like the most efficient way since it reuses code.

Still to do:
 * Unit testing (essentially doing what old unit tests did for storage)
 * Make sure point from comment #11 is checked (not completely sure how relevant it is now)
 * Check what happens on import if master password is canceled

Looking for some more feedback - I still need to add testing before I can think about review.
Attachment #328522 - Attachment is obsolete: true
I have edited the attachment as comment, copied the edited stuff and pasted below (because I didn't know what all those options above submit meant)


>+    log : function (message) {
>+        if (!this._debug)
>+            return;
>+        dump("PwMgr Storage (SQLite): " + message + "\n");
>+        this._logService.logStringMessage("PwMgr Storage (mozStorage): " + message);
>+    },

just for consistency, (SQLite) in dump's arg could be (mozStorage)

>+        let query =
>+            "INSERT INTO moz_logins " +
>+            "(hostname, httpRealm, formSubmitURL, usernameField, " +
>+            "passwordField, encryptedUsername, encryptedPassword) VALUES " +
>+            "(:hostname, :httpRealm, :formSubmitURL, :usernameField, " +
>+            ":passwordField, :encryptedUsername, :encryptedPassword)";
>+
>+        let params = {
>+            hostname: login.hostname,
>+            httpRealm: login.httpRealm,
>+            formSubmitURL: login.formSubmitURL,
>+            usernameField: login.usernameField,
>+            passwordField: login.passwordField,
>+            encryptedUsername: encUsername,
>+            encryptedPassword: encPassword
>+        };
>+
>+        let stmt = this._createStatement(query, params);
>+        try {
>+            stmt.execute();
>+        } catch (e) {
>+            this.log("Error: " + e.name + " : " + e.message);
>+            throw "Couldn't write to database, login not added.";
>+        } finally {
>+            stmt.statement.finalize();
>+        }
>+
>+    },

I think we need a stmt.reset() here.


>+    setLoginSavingEnabled : function (hostname, enabled) {
>+        this.log("setting login saving is enabled for " + hostname + " to " +
>+                 (enabled ? "true" : "false"));
>+        let query;
>+        if (enabled)
>+            query = "DELETE FROM moz_disabledHosts " +
>+                    "WHERE hostname = :hostname";
>+        else
>+            query = "INSERT INTO moz_disabledHosts " +
>+                    "(hostname) VALUES (:hostname)";
>+

wouldn't the else be better as 
        else if (this.getLoginSavingEnabled(hostname))
            query = "INSERT INTO moz_disabledHosts " +
                    "(hostname) VALUES (:hostname)";
        else
            return;


>+
>+        let wrappedStmt = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+                          createInstance(Ci.mozIStorageStatementWrapper);
>+        wrappedStmt.initialize(stmt);
>+
>+        if (params)
>+            for (let i in params)
>+                wrappedStmt.params[i] = params[i];

cannot this simply be (we are in _createStatement function)
        if (params)
            wrappedStmt.params = params;
(In reply to comment #66)
> just for consistency, (SQLite) in dump's arg could be (mozStorage)

Good point, I missed that.

> I think we need a stmt.reset() here.

IIRC we only need to reset after doing SELECT queries (which means I have an extra reset when I'm deleting)

> 
> wouldn't the else be better as 
>         else if (this.getLoginSavingEnabled(hostname))
>             query = "INSERT INTO moz_disabledHosts " +
>                     "(hostname) VALUES (:hostname)";
>         else
>             return;

Good catch, right now it would be inserting duplicates.  I think the better route (to reduce queries) would be to have a UNIQUE on the hostname field in that table. Here's what I changed the table creation to: "hostname           TEXT UNIQUE ON CONFLICT REPLACE" Now we don't have to worry about duplicates.

> cannot this simply be (we are in _createStatement function)
>         if (params)
>             wrappedStmt.params = params;
> 

Actually it can't. I tried that, but the wrappedStmt.params object is not settable, so we have to set each individually. "setting a property that has only a getter" is the error :)


As a side note, I've begun refactoring this to use lazy statement loading. Will post a v0.3 later today.
You should reset anytime you bind a parameter.
Comment on attachment 329000 [details] [diff] [review]
SQLite Patch v0.2

>+    modifyLogin : function (oldLogin, newLogin) {
...
>+        // Since removeLogin will commit a transaction after completion, we add
>+        // the login before removing the old one.
>+        this.addLogin(newLogin);
>+        this.removeLogin(oldLogin);

I'm a bit wary of doing the add before the remove, as it could result in an invalid intermediate state... For example, if we later add consistency checking to the storage module, the addLogin() call might complain because it thinks you're adding a login that already exists. [Look at nsLoginManager.addLogin() for this kind of checking.]

Why would addLogin/removeLogin have different transaction behavior, anyway?

>+        // Just to be sure, we'll commit the transaction
>+        if (this._dbConnection.transactionInProgress)
>+            this._dbConnection.commitTransaction();

This is a few places in the code... But why wouldn't .transactionInProgress always be true (or false)?

>+    _queryLogins : function (whereClause, params) {
...
>+                login.wrappedJSObject.encryptedUsername = stmt.row.encryptedUsername;
>+                login.wrappedJSObject.encryptedPassword = stmt.row.encryptedPassword;

Rather than relying on the .encryptedUsername/Password hack... It would probably be better to just set .username/.password with the encrypted values, with the understanding that the caller will need to decrypt them. [ie, somewhere code will end up doing something like login.username = decrypt(login.username)]
(In reply to comment #67)
> > I think we need a stmt.reset() here.
> 
> IIRC we only need to reset after doing SELECT queries (which means I have an
> extra reset when I'm deleting)
> 
I think u r right. 
Just revisited
http://mxr.mozilla.org/mozilla/source/storage/src/mozStorageStatement.cpp and
http://mxr.mozilla.org/mozilla/source/db/sqlite3/src/sqlite3.c
It appears that reset can be omitted at a few other places too. Not too sure
though!
+        // Loop over ids we're deleting and execute DELETE
+        for each (let id in idsToDelete) {
+            let query = "DELETE FROM moz_logins WHERE id = :id";
+            let params = { id: id };
+            // Create statement
+            let stmt = this._createStatement(query, params);

Instead of the loop, the query could be "DELETE FROM moz_logins WHERE id IN (" + idsToDelete.toString() + ")";
Plus point: a single query 
In continuation to comment #71,
a single query would eliminate the need for all transaction related checks and function calls

The other advantage would be now the modify function can have a begintransaction and commit transaction with add following the remove
(In reply to comment #72)
> In continuation to comment #71,
> a single query would eliminate the need for all transaction related checks and
> function calls
It also means you have to build the statement yourself, and means you probably aren't gonna use the bind* functions, which isn't something you should be doing if it can be avoided.
Errr... There's shouldn't be any loop or multiple logins. Calling removeLogin() should remove exactly one login (matching the one one provided exactly). No more.
(In reply to comment #72)
> In continuation to comment #71,
> a single query would eliminate the need for all transaction related checks and
> function calls
What Shawn said (I talked to him before making that decision to loop).  While what you suggest should work (and is what I wanted originally), the binding is important. And as Justin pointed out, I'm doing it wrong, so it doesn't matter anyway.

(In reply to comment #74)
> Errr... There's shouldn't be any loop or multiple logins. Calling removeLogin()
> should remove exactly one login (matching the one one provided exactly). No
> more.
True, I think I missed the "break" when I was looking at the legacy code and so made this delete multiple since I thought legacy did.  I'll fix this in next rev so it only does a single delete.
Attached patch SQLite Patch v0.3 (obsolete) — Splinter Review
* took into consideration comment #66
* added statement memoization
* removed old, unused methods (createTable, createStatement)
* changed remove_login to only remove a single login.
  - maybe should use DELETE statement as I did in v0.1? (attachment 328522 [details] [diff] [review])
* cleaned up transaction behavior
  - only used in modifyLogin, not removeLogin
  - fixed the order of add/remove
Attachment #329000 - Attachment is obsolete: true
(In reply to comment #69)
> This is a few places in the code... But why wouldn't .transactionInProgress
> always be true (or false)?

It should always be false, with the exception of after .beginTransaction() is called. Maybe I don't need the ifs

> Rather than relying on the .encryptedUsername/Password hack... It would
> probably be better to just set .username/.password with the encrypted values,
> with the understanding that the caller will need to decrypt them. [ie,
> somewhere code will end up doing something like login.username =
> decrypt(login.username)]

I'll look at this a bit more for the next rev. I wanted to get v0.3 up and this will take a little more checking to make sure everybody is in the right encryption state before being passed around.  While I do this, I can clean up ._decryptLogins() and take out the check for mime64-obscured entries, since I'll have to touch the rest of the method.
Attached patch SQLite Patch v0.4 (obsolete) — Splinter Review
Summary of changes since v0.3
* updated to not use login.wrappedJSObject anywhere - just uses username/password as necessary
* removed mime64-obscured stuff from decryptLogins
* removed extraneous comments and commented out code
* added more log messages
* fixed up transaction for modifyLogin

Looking for somebody to look this over, see if anything else jumps out as "bad" or if anything else needs work.  Unit tests are still forthcoming.
Attachment #329157 - Attachment is obsolete: true
Comment on attachment 329908 [details] [diff] [review]
SQLite Patch v0.4

Looking very good! A few small things to fix still, but it's close. Feel free to start on tests. :)

>--- a/modules/libpref/src/init/all.js
...
>+pref("signon.SignonFileName3",              "signons3.txt"); // obselete

obselete?

>+pref("signon.SignonFileName4",              "signons.sqlite");

Why did the pref for the DB come back? (see comment 64)

>+    __dbConnection: null,
>+    get _dbConnection() {
>+        if (!this.__dbConnection)
>+            this.__dbConnection = this._storageService.
>+                                  openDatabase(this._signonsFile);
>+        return this.__dbConnection;
>+    },

Does this need to be a getter? Could _dbConnection just be initialized in init()?

>+    /*
>+     * A list of prefs that have been used to specify the filename for storing
>+     * logins. (We've used a number over time due to compatibility issues.)
>+     * This list is also used by _removeOldSignonsFile() to clean up old files.
>+     */
>+    _filenamePrefs : ["SignonFileName4", "SignonFileName3",
>+                      "SignonFileName2", "SignonFileName"],

SignonFileName4 can go away from here too. Also adjust the comment to indicate that these are only for importing and clearing logins from storage-Legacy.js.

>+        dump("PwMgr Storage (mozStorage): " + message + "\n");
>+        this._logService.logStringMessage("PwMgr Storage (mozStorage): " + message);

Maybe just "PwMgr MozStorage" to keep things brief?

>+    initWithFile : function(aInputFile, aOutputFile) {
>+        this._signonsFile = aInputFile;
>+
>+        this.init();
>+
>+        if (aOutputFile) {
>+            this._signonsFile = aOutputFile;
>+        }
>+    },

Hmm, this won't work the same way as the old code. You can't just switcharoo _signonsFile, as the DB is already open.

This interface is really just for the convenience of tests, so we can adjust the semantics a bit...

* aInputFile becomes aImportFile. If specified, make this used for feeding storage-Legacy.
* aOutputFile becaomes aDBFile. If set, use this instead of $profile/signons.sqlite.

Tests can then do things like (pseudocode):
initWithFile("signons3-test1.txt", "signons-test1.sqlite") // check importing
initWithFile(null, "signons-test2.sqlite"); // check output of last run
initWithFile("signons-test3.txt", null); // check importing to default DB

What we *can't* do is something like:
initWithFile("test1-input.sqlite", "test1-output.sqlite");

Current tests do something like that that just as a short-cut for treating test1-input.txt as a read-only master. Instead, if the DB tests want to do something like that they'll need to do:

cp test1-input.sqlite test1-output.sqlite
initWithFile(null, "test1-output.sqlite");

>+        // If we have an import file, do a switcharoo before reading it.
>+        if (importFile) {

Definitely need to have some sort of testcase to check what happens when the user doesn't enter a master password for importing. If we can detect this case (which might be tricky, I don't think getAllLogins() throws when the MP isn't entered?), we can throw here, which causes nsLoginManager's _storage getter to not be initialized, and so the process will just repeat itself the next time the user triggers the login manager, unless they finally relent and enter their MP. I *think* that should work.

>+            let legacy = Cc["@mozilla.org/login-manager/storage/legacy;1"].
>+                            createInstance(Ci.nsILoginManagerStorage);

Wrong indent! :)

>+    addLogin : function (login) {
...
>+        // Get the encrypted value of the username and password.
>+        // Assume that all incoming logins are unencrypted.

Last comment is unneeded, the incoming logins are unencrypted by definition.

>+        let params = {
>+            hostname: login.hostname,
>+            httpRealm: login.httpRealm,
>+            formSubmitURL: login.formSubmitURL,
>+            usernameField: login.usernameField,
>+            passwordField: login.passwordField,
>+            encryptedUsername: encUsername,
>+            encryptedPassword: encPassword
>+        };

Nit: indent the values to  the "login." bits all line up.

>+        try {
>+            var stmt = this._dbCreateStatement(query, params);

"let" is the new "var"!

>+    removeLogin : function (login) {
>+        // First query for logins matching everything else
>+        let [logins, ids] = this._queryLogins("WHERE hostname = :hostname",
>+                                                 { hostname: login.hostname });

This should go ahead and query for every nsILoginInfo field (except for the 2 encrypted fields).

The legacy code had to check for just the hostname first, because logins were stored in memory hashed by the hostname. Technically the legacy code could then whittle down that list by throwing out logins that didn't match the other fields (which you'd be doing in this query), but it didn't seem like it was worth the extra work in the old code.


>+        // The specified login isn't encrypted, so we need to ensure
>+        // the logins we're comparing with are decrypted. We decrypt one entry
>+        // at a time, lest _decryptLogins return fewer entries and screw up
>+        // indices between the two.
>+        for (let i = 0; i < logins.length; i++) {
>+
>+            let [[decryptedLogin], userCanceled] =
>+                        this._decryptLogins([logins[i]]);

This won't be an issue in the DB code. You can just call this._decryptLogins(logins) to decrypt them all, then loop to match the username/password, and delete that one by ID.

The legacy code had to do this step-by-step, to ensure that offsets in array of decrypted logins were the same as offsets as in the in-memory array of encrypted logins. Not relevant here.

>+    modifyLogin : function (oldLogin, newLogin) {
...
>+        if (this._dbConnection.transactionInProgress)
>+            throw "Transaction already in progress";

Was this still needed here?

>+        // Begin a transaction to wrap remove and add
>+        this._dbConnection.beginTransaction();
>+
>+        // Wrap add/remove in try-catch so we can rollback on error
>+        try {
>+            this.removeLogin(oldLogin);
>+            this.addLogin(newLogin);
>+        } catch (e) {
>+            this._dbConnection.rollbackTransaction();
>+        }
>+
>+        // Commit the transaction
>+        this._dbConnection.commitTransaction();

If an exception was caught there, rethrow it after you rollback.

>+    getAllLogins : function (count) {
>+        let userCanceled;
>+
>+        let [result, ids] = this._queryLogins(null, {});
>+
>+        // decrypt entries for caller.
>+        [result, userCanceled] = this._decryptLogins(result);

Hmm, I guess JS complains about var1 if you do:

let [var1, var2] = xxx
let [var3, var1] = yyy

>+    removeAllLogins : function () {
...
>+        // Disabled hosts kept, as one presumably doesn't want to erase those.
>+        // Or should we follow old behavior here?

Yes, we want to (and are) doing what the old code did. This quizzical comment can be removed. :)


>+    /*
>+     * getLoginSavingEnabled
>+     *
>+     */
>+    getLoginSavingEnabled : function (hostname) {
>+        this.log("Setting login saving is enabled for " + hostname);

s/Setting/Getting/

>+    countLogins : function (hostname, formSubmitURL, httpRealm) {
>+        let logins = this._searchLogins(hostname, formSubmitURL, httpRealm);
>+        this.log("countLogins: counted logins: " + logins.length);
>+        return logins.length;
>+    },

This code is slightly performance sensitive -- fillDocument() calls it to bail out early when there are no stored logins for a host. It would be interesting to measure the call time is in this case, and see if a specialized query is any faster (or not worthwhile).

>+    _queryLogins : function (whereClause, params) {
>+        this.log("_QueryLoginInfo(" + whereClause + "," + params.toSource() +")");

nit: make the log message match the function name (caps).


>+    _getSignonsFile : function() {

This function shouldn't be needed any more, since the signon file isn't pref'd.

filenamePrefs can then be made local to removeOldSignonsFiles(), I believe.


>+}; // end of nsLoginManagerStorage_sqlite implementation

nit: it's _mozStorage now.
(In reply to comment #79)

> >+        try {
> >+            var stmt = this._dbCreateStatement(query, params);
> 
> "let" is the new "var"!

I used "var" here because I still need scope in the finally. I might be able to do it slightly differently, but since _dbCreateStatement can throw, I figured it was best to put the assignment into the try.

> >+    modifyLogin : function (oldLogin, newLogin) {
> ...
> >+        if (this._dbConnection.transactionInProgress)
> >+            throw "Transaction already in progress";
> 
> Was this still needed here?

I wasn't sure. I thought it was being safe and covering our bases.  Though since I'm not doing it elsewhere and it's pretty unlikely (impossible?) to call modifyLogin again before it's done, I can probably get rid of it

I've already fixed all of the easier stuff here, just need to fix the importing and prefs stuff.
Status: NEW → ASSIGNED
Product: Firefox → Toolkit
Attached patch SQLite Patch v0.5 (obsolete) — Splinter Review
Lots of changes (talked with dolske about most of these). Highlights:
* overhaul of initialization and importing
* some work happens in init, most in _deferredInit
* requires check for correct init, so all public methods now call a private function, which does all the work
* no longer an extra pref

There may still be some excess log messages and error throwing. There will be a final run through before final patch to check on status of those.

There is one TODO remaining, and it has to do with failed migrations. Since that won't be happening yet, I've left the TODO comment in. Should be pretty easy when the need arises to match against the thrown error, as is being done already in _dbInit
Attachment #329908 - Attachment is obsolete: true
Attached patch SQLite Patch v0.6 (obsolete) — Splinter Review
Summary:
* Tests! - mostly converted the old tests. added in a couple new ones, made them use mozStorage
* fixes a few errors uncovered by bugs

Notes:
* _importLegacySignons is still called from 2 places (_dbInit and _deferredInit). This isn't ideal, but it's the only way to do this properly and still handle import failure errors correctly. The primary case is the call in _dbInit. _deferredInit only handles when initWithFile is called (which is primarily for testing).
* I cleaned up log messages, so there shouldn't be any extraneous ones
* I think the error throwing we're doing is ok and won't break anything.

Changes:
M modules/libpref/src/init/all.js
M toolkit/components/passwordmgr/src/Makefile.in
M toolkit/components/passwordmgr/src/nsLoginManager.js
A toolkit/components/passwordmgr/src/storage-mozStorage.js
M toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js
A toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_1.js
A toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_2.js
A toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_3.js
A toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_4.js
A toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_5.js
Attachment #332278 - Attachment is obsolete: true
Attachment #333014 - Flags: review?(dolske)
Comment on attachment 333014 [details] [diff] [review]
SQLite Patch v0.6

Overall: excellent work! I'm going to r- just because there's a sizable number of little things that need tweaking, but this is close to being ready!


>+    __ioService: null, // IO service for string -> nsIURI conversion

Looks like ioService isn't used now, so this can be removed.
>+    _signonsFile  : null,  // nsIFile for "signons.sqlite" (or whatever pref is)

No pref for this, so last 1/2 of comment can go away.

>+    _initializing : false, // check state so we don't get out of sync

That comment seems a little confusing. Isn't this just to prevent concurrent initializations?

>+    initWithFile : function(anImportFile, aDBFile) {

While this is nicely grammatically correct, the "a" is for "argument", so "aImportFile" should be used. :)

>+    _deferredInit : function () {
>+        // Check that we are not already in and initializing state
>+        if (this._initializing) {
>+            this.log("Already initializing, returning");
>+            return;
>+        }


Briefly note in the function header that canceling a master-password entry is the specific reason this might fail (or be reentered), since that's a bit unexpected.

You're checking this._initializing both here and in checkInitializationState (which is the only caller)... I think this should be a throw here, and don't bother checking _initializing in checkInitializationState.

Please move checkInitializationState() to after deferredInit, so the code is close together.

>+            // Initialize the database (create, migrate as necessary)
>+            this._dbInit();
>+
>+            // If we have an import file, import it
>+            if (this._importFile)
>+                this._importLegacySignons(this._importFile);

I think it would be clearer to have _dbInit return a isFirstRun flag, and then do

if (isFirstRun)
    this._importLegacySignons(this._importFile);

(If we're not in initWithFile, _importFile will be null).

Would it be difficult to make the storage module not create the on-disk file unless something is stored? I suppose some time in the future bug 449095 will apply to this module. :-) Maybe we can just fix it then.

>+        } catch (e) {
>+            this.log("Initialization failed");
>+            this._initialized = false;
>+        }

Also set this._initializing to false in the catch (or be fancy and use a finally-block).

I don't think the catch needs to set _initialized to false, because it can't get there if it's true. Also, the catch should just rethrow the exception, so that checkInitializationState() doesn't need to see if it succeeded.

>+    /*
>+     * addLogin
>+     *
>+     */
>+    addLogin : function (login) {
>+        // Check initialization state & try to init again, throws on failure.
>+        // Needed at beginning of *every* public function.

That's not actually true. These mini-wrappers are only needed for addLogin and setLoginSavingEnabled, because they're the only functions that need to be callable from _importLegacySignons. The other public interfaces don't need to be wrapped like this.

>+    _addLogin : function (login) {
...
>+        let query =
>+            "INSERT INTO moz_logins " +

What happens here if the login already exactly matches an existing entry?

>+        try {
>+            var stmt = this._dbCreateStatement(query, params);

var? VAR?! let is the new var!

There are 6 other "var"s in this patch. :)

>+        } catch (e) {
>+            this.log("Error: " + e.name + " : " + e.message);

The context won't always be clear for this message, better to have:

  this.log("addLogin failed: " +
           e.name + ....

Ditto for the other functions where errors from executing the statement are logged.

>+     _removeLogin : function (login) {
...
>+         // Do checks for null and empty strings, adjust query and params
>+         // login.hostname *should* always be set, so will be in conditions
>+         [conditions, params] =
>+             this._adjustConditionsAndParams(conditions, params, login.hostname,
>+                                        login.formSubmitURL, login.httpRealm);

adjustConditionsAndParams() is probably a little too loose here... If login.formSubmitURL or .httpRealm is an empty-string, it might end up match a different login.

Oh. Hmm. I see, you're then looping over the results and using .equals, so that's ok.

Can you just use _searchLogins() here (modifying it to return ids too)? Since it's going to iterate over the logins for an exact match anyway, the query doesn't have to match against the usernameField / passwordField.

>+         // Query for logins matching everything but encrypted fields
>+         let [logins, ids] = this._queryLogins(conditions, params);
>+
>+         // The specified login isn't encrypted, so we need to ensure
>+         // the logins we're comparing with are decrypted.
>+         let [decryptedLogins, userCanceled] = this._decryptLogins(logins);

You'll need to decrypt 1-by-1, as in legacy storage, because decryptLogins() will skip corrupted entries. This would result in ids.length != decryptedLogins.length, so the wrong login could get removed.

This is probably less of a concern with the sqlite file, so it might be ok to just check if decryptedLogins.length == logins.length, and throw if they differ... Hmm, but "corrupted" entries could also be due to someone changing key3.db manually in the profile, so we better just do 1-by-1 for now. :/

>+         for (let i = 0; i < decryptedLogins.length; i++) {
>+             // Remove decryptedLogins[i] and break from loop if matches login
>+             if (decryptedLogins[i].equals(login)) {

Nit: |if (!equal) continue;| lets you skip having everything in a big indented block.

We can avoid having a return buried in this block by having the output of the loop just be an ID to delete (and throw if we can't match one), that also allows the query execution to pulled out from the loop and not indented as much.

>+    _modifyLogin : function (oldLogin, newLogin) {
...
>+
>+        if (this._dbConnection.transactionInProgress)
>+            throw "Transaction already in progress";
>+
>+        // Begin a transaction to wrap remove and add
>+        this._dbConnection.beginTransaction();

What happens if beginTransaction() is called when a transaction is already in progress?

>+    _getAllLogins : function (count) {
>+        let userCanceled;
>+
>+        let [result, ids] = this._queryLogins([], {});
>+
>+        // decrypt entries for caller.
>+        [result, userCanceled] = this._decryptLogins(result);

This should throw if userCanceled (sync with bug 449810).

>+    _setLoginSavingEnabled : function (hostname, enabled) {
...
>+        this.log("Setting login saving is enabled for " + hostname + " to " +
>+                 (enabled ? "true" : "false"));

|log("... to " + enabled)| is sufficient.

>+    _countLogins : function (hostname, formSubmitURL, httpRealm) {
>+        let logins = this._searchLogins(hostname, formSubmitURL, httpRealm);
>+        this.log("_countLogins: counted logins: " + logins.length);
>+        return logins.length;
>+    },

This function is mildly perf sensitive, because it's getting called on every page load. The query here is probably ok, though, because the interesting case is when there are *no* logins stored for the page being visited.

Can you kick off a tryserver build with this patch, just so we know if there's any measurable perf impact?

>+    _searchLogins : function (hostname, formSubmitURL, httpRealm) {
>+        // Do checks for null and empty strings, adjust query and params
>+        let [conditions, params] =
>+            this._adjustConditionsAndParams([], {}, hostname, formSubmitURL, httpRealm);

This will now be the only caller of adjustConditionsAndParams (assuming removeLogin can be adjusted to call searchLogins)... So that code can just be inlined here.

>+    _queryLogins : function (conditions, params) {
...
>+            for (let i = 0; i < conditions.length; i++)
>+                conditions[i] = "(" + conditions[i] + ")";
>+            query += " WHERE " + conditions.join(" AND ");

let blah = conditions.map(function(c) "(" + c + ")");
query += " WHERE " + blah.join(" AND ");

>+                login.init(stmt.row.hostname, stmt.row.formSubmitURL,
>+                           stmt.row.httpRealm, "", "",
>+                           stmt.row.usernameField, stmt.row.passwordField);
>+                login.username = stmt.row.encryptedUsername;
>+                login.password = stmt.row.encryptedPassword;

Why treat username/password specially?


>+    _adjustConditionsAndParams : function (conditions, params, hostname, formSubmitURL, httpRealm) {
>+        if (hostname == null) {
>+            conditions.push("hostname isnull");

Hostname should never be null, so this case isn't needed. Oh, hmm, nevermind. I guess the legacy module supported this just for consistency's sake.

>+    _checkLoginValues : function (aLogin) {

Resync this with bug 449698.

>+            // Import logins and disabledHosts
>+            let logins = legacy.getAllLogins({});
>+            for each (let login in logins)
>+                this._addLogin(login);

Hmm. If addLogin fails because the user canceled the master password entry, we want defer the import (as you do now). But if addLogin throws because the login's format is invalid (which might happen if they had old pre-existing logins stored, before we started checking), then we want to skip that login and continue with the import.

>+    /*
>+     * _decryptLogins
...
>+     *         This is deferred from the login query code, so that
>+     * the user is not prompted for a master password (if set) until
>+     * the entries are actually used.

This comment can just go away, it's not really relevant now.

>+    _decryptLogins : function (logins) {
...
>+            // We could set the decrypted values on a copy of the object, to
>+            // try to prevent the decrypted values from sitting around in
>+            // memory if they're not needed. But thanks to GC that's happening
>+            // anyway, so meh.

This comment also can go away now; not relevant.

>+    _decrypt : function (cipherText) {
...
>+            if (cipherText.charAt(0) == '~') {
>+                // The older file format obscured entries by
>+                // base64-encoding them. These entries are signaled by a
>+                // leading '~' character.

Don't need to support that format now, so this code can be removed.

>+    _dbCreateStatement : function (query, params) {
>+            try {
>+                var stmt = this._dbConnection.createStatement(query);
>+            } catch (e) {
>+                this.log("Error: " + e.name + " : " + e.message);
>+                throw "Error creating statement";
>+            }

Perhaps not bother with the try/catch/log here? All the callers already are wrapping this in a try/catch, so they'll detect and log failures (with more context, too).

>+            this.log("Reusing statement for query: " + query);

This seems unneeded, as it'll just contribute to log spew.


>+    // _dbInit and the methods it calls (_dbCreate, _dbMigrate, and version-
>+    // specific migration methods) must be careful not to call any method
>+    // of the service that assumes the database connection has already been
>+    // initialized, since it won't be initialized until at the end of _dbInit.
>+
>+
>+    /*
>+     * _dbInit

Merge these comments together?

>+        } catch (e) {
>+            if (e == "Import failed") {
>+                this.log("_dbInit failed, closing & deleting database");
>+                for (let i = 0; i < this._dbStmts.length; i++) {
>+                    this._dbStmts[i].statement.finalize();
>+                    this._dbStmts[i] = null;
>+                }
>+                this._dbConnection.close();
>+                this._dbConnection = null;
>+                this._signonsFile.remove(false);
>+                throw(e);

Semi-random thought...

It might be more better to get a list of logins/hosts to import, then create the DB, and then add the imported logins. Two benefits:

* More robust against failures. If we can't get the import logins, we don't have to delete the DB. (If something failed in this process, the empty DB would prevent re-importing next time 'round).

* Helps with previous comment about aborting when calling legacy.getAllLogins(), but just skipping bad logins when calling self.addLogin(aLegacyLogin).



Next up: review pass of tests.
Attachment #333014 - Flags: review?(dolske) → review-
(In reply to comment #83)
> Would it be difficult to make the storage module not create the on-disk file
> unless something is stored? I suppose some time in the future bug 449095 will
> apply to this module. :-) Maybe we can just fix it then.

That would be nice, but it'll take a bit more work here than it did in legacy
storage.

> I don't think the catch needs to set _initialized to false, because it can't
> get there if it's true. Also, the catch should just rethrow the exception, so
> that checkInitializationState() doesn't need to see if it succeeded.

I opted to throw a consistent error, instead of whatever comes through, but
other "advice" taken.


> What happens here if the login already exactly matches an existing entry?

That's a very good question. As of now I'm pretty sure this behaves the same as
the legacy module and just inserts a new one. We could add in checks to ensure
that doesn't happen. It might not be too much more work (and might just be able
to add a UNIQUE INDEX into the db can catch that case). Should we cover that
case?

> What happens if beginTransaction() is called when a transaction is already in
> progress?

Sean says it will throw, so either way we end up throwing. Should I skip the
check and just call it regardless?

> Hmm. If addLogin fails because the user canceled the master password entry, we
> want defer the import (as you do now). But if addLogin throws because the
> login's format is invalid (which might happen if they had old pre-existing
> logins stored, before we started checking), then we want to skip that login and
> continue with the import.

I changed this to wrap it in a try catch to mitigate, but it's not as pertty as
I would like.


> Semi-random thought...
> 
> It might be more better to get a list of logins/hosts to import, then create
> the DB, and then add the imported logins. Two benefits:
> 
> * More robust against failures. If we can't get the import logins, we don't
> have to delete the DB. (If something failed in this process, the empty DB would
> prevent re-importing next time 'round).
> 
> * Helps with previous comment about aborting when calling
> legacy.getAllLogins(), but just skipping bad logins when calling
> self.addLogin(aLegacyLogin).

This last comment should be ok now, so we'd really just be saving a file
creation & destruction. That would be good to get rid of, but that could be
done in a future bug to bring this in sync with bug 449095
>> What happens here if the login already exactly matches an existing entry?

> That's a very good question. As of now I'm pretty sure this behaves the same as
> the legacy module and just inserts a new one. We could add in checks to ensure
> that doesn't happen. It might not be too much more work (and might just be able
> to add a UNIQUE INDEX into the db can catch that case). Should we cover that
> case?

This is a known problem in the old wallet code (Bug 293887). Nobody is fixing it because we (SeaMonkey) are moving to satchel/nsLoginManager RSN. I certainly hope that such a check is included in the new code.
(In reply to comment #84)

> > I don't think the catch needs to set _initialized to false, because it can't
> > get there if it's true. Also, the catch should just rethrow the exception, so
> > that checkInitializationState() doesn't need to see if it succeeded.
> 
> I opted to throw a consistent error, instead of whatever comes through, but
> other "advice" taken.

Ok, your upcoming patch looks like it addressed my concern anyway.


> > What happens here if the login already exactly matches an existing entry?
> 
> That's a very good question. As of now I'm pretty sure this behaves the same as
> the legacy module and just inserts a new one. We could add in checks to ensure
> that doesn't happen. It might not be too much more work (and might just be able
> to add a UNIQUE INDEX into the db can catch that case). Should we cover that
> case?

That's probably ok (bad answer is DB corruption or something like that :). It's up to the login manager to filter out duplicates, and the module itself can't really check for duplicates without decrypting the username/password anyway.

> > What happens if beginTransaction() is called when a transaction is already in
> > progress?
> 
> Sean says it will throw, so either way we end up throwing. Should I skip the
> check and just call it regardless?

Yeah, if it's going to throw there's no need to explicitly check for the condition. Doesn't look like it's really possible for that to happen anyway.


> > Hmm. If addLogin fails because the user canceled the master password entry, we
> > want defer the import (as you do now). But if addLogin throws because the
> > login's format is invalid (which might happen if they had old pre-existing
> > logins stored, before we started checking), then we want to skip that login and
> > continue with the import.

Ok, this can be ignored. It's unlikely to happen in the first place, and you'll never actually be able to get invalid logins from the legacy module anyway -- the bogus characters mess with the file formatting, not the returned values. [Eg, newlines would never surive the line-based processing.]
(In reply to comment #85)

> This is a known problem in the old wallet code (Bug 293887). Nobody is fixing
> it because we (SeaMonkey) are moving to satchel/nsLoginManager RSN. I certainly
> hope that such a check is included in the new code.

The new login manager already handles this checking in common code (nsLoginMananger.js's addLogin method). The storage modules themselves thus don't need to, and really can't without wading into decryption issues. 

Attached patch SQLite Patch v0.7 (obsolete) — Splinter Review
Fixed everything from comment 83 (minus the points I brought up in comment 84 and discussed further).
Attachment #333014 - Attachment is obsolete: true
Attachment #333353 - Flags: review?(dolske)
Will we need changes to get SeaMonkey/Thunderbird passwords that are still wallet currently migrated to this mozStorage DB (i.e. different changes than what migrates to the current textfile)?
SeaMonkey/TB will get this for free -- the mozStorage module uses the existing ("legacy") storage module for importing existing logins, and  the legacy module already handles the format conversions SM/TB need.
Dolske, thanks for the comment, that's what I hoped to hear :)
Comment on attachment 333353 [details] [diff] [review]
SQLite Patch v0.7

Requesting review from sdwilsh on the mozStorage parts.
Attachment #333353 - Flags: review?(sdwilsh)
Summary: convert signons2.txt to mozstorage → convert signons3.txt to mozstorage
Target Milestone: --- → mozilla1.9.1a2
Blocks: 316084
Comment on attachment 333353 [details] [diff] [review]
SQLite Patch v0.7

>+    _deferredInit : function () {
...
>+        } catch (e) {
>+            this.log("Initialization failed");
>+            // If the import fails on first run, we want to delete the db
>+            if (isFirstRun && e == "Import failed") {
>+                for (let i = 0; i < this._dbStmts.length; i++) {
>+                    this._dbStmts[i].statement.finalize();
>+                    this._dbStmts[i] = null;
>+                }
>+                this._dbConnection.close();
>+                this._dbConnection = null;
>+                this._signonsFile.remove(false);
>+            }

This is a slightly surprising amount of cleanup work... Do the statements really need .finalize called? Maybe just set |this.dbStmts = []|?


>+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_1.js
...
>+/* ========== 1 ========== */
>+var testnum = 1;
>+var testdesc = "Initial connection to storage module"
>+
>+var storage = LoginTest.newMozStorage();
>+LoginTest.deleteFile(OUTDIR, "signons.sqlite");

Add a test here to verify that the file exists.

>+testdesc = "Initialize with signons-06.txt (1 disabled, 1 login); test removeLogin";
...
>+testdesc = "Initialize with signons-06.txt (1 disabled, 1 login); test modifyLogin";

Add these additional tests to the legacy storage tests too, since they're lacking this coverage.


>+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_2.js
...
>+/* ========== 10 ========== */
>+testnum++;
>+dump("-----------------++++++++++++________-------\n")

Please don't check in EBay comments. :)


General comment about the tests, for my future reference... There's a fair amount of duplicated storage-Legacy test code, which in some sense shouldn't be needed -- if the legacy module imports and does format conversions as it's expected to (and the existing tests verify), the mozStorage code shouldn't need to exercise it again. But I think this does add some value, for a couple of reasons:

1) It's exercising the legacy code in the context it will actually be used.
2) It's verifying that these logins, in the various edge-case formats they come it, are properly handled by the mozStorage.

If we want to make these copied tests Perfect, we could probably reduce them down to just calling mozStorage.addLogin() in the various formats. Maybe we'll do that if they become unwieldy, but I'm content right now to take the easy path that was done here.
Attachment #333353 - Flags: review?(dolske) → review+
Comment on attachment 333353 [details] [diff] [review]
SQLite Patch v0.7

r=sdwilsh, with comments on paper, handed to zpao
Attachment #333353 - Flags: review?(sdwilsh) → review+
Attached patch SQLite Patch v0.8 (obsolete) — Splinter Review
Took above reviews, and fixed. Should be final.

Biggest change - if the database gets corrupted we back it up, delete the original, then next time login manager gets used, we create a new db - and added a test for this.

Also, created a new method _dbCleanup since there are now two paths to removing a database file
Attachment #333353 - Attachment is obsolete: true
Comment on attachment 333651 [details] [diff] [review]
SQLite Patch v0.8

driveby comment:

>+    __decoderRing : null,  // nsSecretDecoderRing service
>+    get _decoderRing() {
>+        if (!this.__decoderRing)
>+            this.__decoderRing = Cc["@mozilla.org/security/sdr;1"].
>+                                 getService(Ci.nsISecretDecoderRing);
>+        return this.__decoderRing;
>+    },

there's a better way to memoize this stuff (function overhead sucks, this is cleaner to read and way faster):

get _decoderRing() {
  delete this._decoderRing;
  return this._decoderRing = Cc["@mozilla.org/security/sdr;1"].
                             getService(Ci.nsISecretDecoderRing);
}
Attached file Memoization testcase
(In reply to comment #96)

> there's a better way to memoize this stuff (function overhead sucks, this is
> cleaner to read and way faster):
> 
> get _decoderRing() {
>   delete this._decoderRing;
>   return this._decoderRing = Cc["@mozilla.org/security/sdr;1"].
>                              getService(Ci.nsISecretDecoderRing);
> }

Doesn't work, at least in a prototype.

This attachment throws "Error: setting a property that has only a getter" at line 19 (return this.bar = "222";).

Gavin and sdwilsh have also mentioned this format -- which I like! -- but I don't see a good way to use it. Gavin suggested it might be possible to define the getter in the object's constructor, but that seems kind of ugly. :/
Attached patch SQLite Patch v0.9 (obsolete) — Splinter Review
Improved countLogins performance. Now it's final?
Attachment #333651 - Attachment is obsolete: true
Comment on attachment 333843 [details] [diff] [review]
SQLite Patch v0.9

A few comments on the changes between patch v0.7 and v0.9...

>+    // The current database schema
>+    _dbSchema: {
...
>+          moz_logins_hostname_formSubmitURL_index: {
>+            table: "moz_logins",
>+            columns: ["hostname", "formSubmitURL"]
>+          }

Let's go ahead an index on hostname+httpRealm too. (We've flip-flopped after discussing this with Shawn, I think current understanding is that although it'll use a little bit more space on disk, testing has shown that the perf benefits can make it worthwhile.)


>+    countLogins : function (hostname, formSubmitURL, httpRealm) {
>+        this._checkInitializationState();
>+
>+        let [logins, ids] = this._searchLogins(hostname, formSubmitURL, httpRealm, false);

Add a comment here that _searchLogins will only returning ids.

(Could use |let [,ids] = ...|, but I tend to think that syntax makes it look like someone forgot to type something).

>+    /*
>+     * _searchLogins
>+     *
>+     * Returns array of [logins, ids]
>+     */
>+    _searchLogins : function (hostname, formSubmitURL, httpRealm, initLogins) {

Add a brief comment here (and to _queryLogins) describing the extra initLogins arg.

>+    _queryLogins : function (conditions, params, initLogins) {
...
>+                if (!initLogins)
>+                    continue;

I think this would read better with a different parameter name: |if (countOnly) continue;|. You'll have to flip the boolean in callers, too.

w00t, almost done!
I did some testing in a modified version of the Standalone Talos, to address some deficiencies in the existing Tp perf test. The modifications add pages with login forms, and add logins to the testing profile.

The nutshell conclusion is that the mozStorage module is equal-to or faster-then the legacy module in all of the tests. See attachment for specific numbers and breakdown. (Test modifications coming in a separate attachment)
Attached file Talos modifications
These are the modifications to Standalone Talos.

When running, you'll need to copy signons-[big|small].txt to signons3.txt in talos/base_profile/, depending on which set of logins to test with. Delete any signons3.txt to test with no signons.

Also, Paul will soon be posting some additional benchmarks that exercise the other storage module APIs.
Attached file xpcshell perf "tests" (obsolete) —
A zip file containing the xpcshell "test", signons-08.sqlite, and results.

Drop test_storage_speed.js into /toolkit/components/passwordmgr/test/unit, and copy signons-08.sqlite into objdir/dist/bin, and run the test.

The results show pretty much the same as Justin's. countLogins might be marginally slower, with the difference being milliseconds. Initialization is ~30x faster, and other critical methods are on par or 2-20x faster. This was done with a large dataset.
Changes based on comment #99
Attachment #333843 - Attachment is obsolete: true
Comment on attachment 334021 [details] [diff] [review]
SQLite Patch v1.0

Looks good, will be landing this.
Attachment #334021 - Flags: review+
Attached file xpcshell perf "tests"
Updated "tests" - includes perf results for more methods, and now tests the case with a small initial file under more typical activity.

To use, copy .js files to toolkit/components/passwordmgr/test/unit, .txt files to toolkit/components/passwordmgr/test/unit/data, .sqlite files to objdir/dist/bin and run.
Attachment #333896 - Attachment is obsolete: true
Pushed changeset c2f416981fa3. Yay -- great work, Paul!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted1.9.1?
Resolution: --- → FIXED
Flags: in-testsuite+
Just a question. When will the file signons.sqlite be created? Aren't getting existing entries in signons3.txt converted to the sqlite database? I run a current nightly build but this file doesn't exists in the used profile.
Attached patch Test fixesSplinter Review
This had a bumpy landing on Friday -- the tests had issues on Windows, and so I disabled the mozStorage switch while looking into the tests.

We don't (can't) explicitly close the DB when we finish testing with a storage module instance, and removing an open file fails on Windows. The tests have been modified to (1) ignore file deletion failures when cleaning up after a test [we could just not delete at all, but it's nice to stay clean mid-run on other platforms if possible] and (2) delete the output file before starting a test (w/o ignoring failures).

Tests pass on Windows and OS X with these changes, so I've landed them and reenabled mozStorage for password manager.
With today's nightly build, passwords have disappeared altogether. 

See:
http://forums.mozillazine.org/viewtopic.php?p=4198435#p4198435 

I still have a signons3.txt file also..
This needs to be backed out ASAP until it can be properly implemented. It is apparent that other aspects of the browser needed to be revised to handle this change so that, for example, Tools > Options > Security > Passwords > Saved Passwords would know where to "look" to show our logins/passwords or where to "look" for our logins/passwords on login pages.

Very disappointing that this was handled in such a poor way.
(In reply to comment #110)
> This needs to be backed out ASAP until it can be properly implemented. It is
> apparent that other aspects of the browser needed to be revised to handle this
> change so that, for example, Tools > Options > Security > Passwords > Saved
> Passwords would know where to "look" to show our logins/passwords or where to
> "look" for our logins/passwords on login pages.
> 
> Very disappointing that this was handled in such a poor way.
> 

It is apparent that whatever conversion was supposed to take place with this implementation did not happen.
OK, you've been around long enough to know that Bugzilla is not the place to be whining about this. Please keep your complaints to the forums and stop spamming everyone CCed to this bug.
Justin, I did some test and have a question to the backward compatibility. If users switching back to Firefox 3.0.x for a while and returning to Firefox 3.1 all their newly added passwords aren't converted to mozstorage because we do that only if signons.sqlite doesn't exist. In such a situation the user has to manually delete the signons.sqlite which could also probably raise dataloss. Shouldn't we better sync signons3.txt and signons.sqlite based on their latest access time in Firefox 3.1? Shall I file a new bug on that?
(In reply to comment #113)
> Justin, I did some test and have a question to the backward compatibility. If
> users switching back to Firefox 3.0.x for a while and returning to Firefox 3.1
> all their newly added passwords aren't converted to mozstorage because we do
> that only if signons.sqlite doesn't exist. In such a situation the user has to
> manually delete the signons.sqlite which could also probably raise dataloss.
> Shouldn't we better sync signons3.txt and signons.sqlite based on their latest
> access time in Firefox 3.1? Shall I file a new bug on that?

Can this patch be applied to Firefox 3.0.x or is it too complex? If it can, then it would address Henrik's concerns.
(In reply to comment #113)
> Justin, I did some test and have a question to the backward compatibility. If
> users switching back to Firefox 3.0.x for a while and returning to Firefox 3.1

We've never supported that for other types of data (eg, bookmarks), and won't here either.

(In reply to comment #114)
> Can this patch be applied to Firefox 3.0.x

No.
Marking verified fixed. No dependencies anymore and everything works. Thanks Paul!
Status: RESOLVED → VERIFIED
Comment on attachment 334021 [details] [diff] [review]
SQLite Patch v1.0

s/existant/existent/g
You need to log in before you can comment on or make changes to this bug.