468180 - Passwords should be reimported into signons.sqlite if signons3.txt is newer

Status: VERIFIED WONTFIX

(Toolkit :: Password Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2

After having a talk on IRC we noticed that the switch from signons3.txt to signons.sqlite in Firefox 3.1 will definitely cause trouble for a lot of users. There was a similar reaction with the bookmark changes for Firefox 3. Cww could give more statistics here.

So what can happen? As we already know the amount of beta testers is overwhelming. So everyone who is trying out the beta version or a release candidate could be affected by this behavior, when no new profile was created for testing purposes. Running the beta version for the first time an existing signons3.txt will be converted into the mozstorage format. That's fine but how long will users test the beta version? Normally they will switch back to their daily browser which could be Firefox 3.0 and will wait for the final release. Within this time they have the chance to enter more passwords to their signons3.txt. Everything will be fine until the final version of Firefox 3.1 will be available. Doing the upgrade and restarting the browser will cause a loss of all entered passwords between testing the beta version and now. That happens due to the import isn't started again. It only happens once - when no signons.sqlite can be found. Later changes of signons3.txt aren't recognized anymore.

As we have seen for the bookmark issue thousands of users will be lost for the first time and are offended why their passwords are gone. A link like

http://support.mozilla.com/en-US/kb/Firefox+3+upgrade+switches+bookmarks+and+other+settings+to+older+version

can help to solve the issue for them but shouldn't we try to do that automagically? Checking the last modification time of both files and starting an import again if the signons3.txt is newer? Or at least giving it a pref like we do for the bookmark import.

Adding some ppl who could hopefully give more comments to show the importance of this bug.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

There's a risk associated with using beta software.

I think having a page on sumo is fine. The method for forcing the reimport is the same as the linked bookmarks (though obviously with a different file).

I don't think we should reimport automagically, but maybe that's just me.

Comment 2

[Justin Dolske [:Dolske]]

We don't do this with other data types (notably, the switch to places.sqlite in FF3), as it's a hairy problem. WONTFIX.

Comment 3

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

(In reply to comment #2)
> We don't do this with other data types (notably, the switch to places.sqlite in
> FF3), as it's a hairy problem. WONTFIX.

I don't think we should be so quick to dismiss this. The switch to places.sqlite caused a lot of support headaches, and if we can do a better job here we should at least try.

What are the problems with importing passwords from a recently modified signons.txt non-destructively?

Comment 4

[Justin Dolske [:Dolske]]

* We don't store per-entry timestamps, so if a FF3.1 user ever goes back and runs FF2/3 just once and does _anything_ to modify the file (save a login, save an exception, delete a login, etc), we'd be importing all their ancient signons again. [Note that we don't delete signons3.txt after importing.]

* Ambiguity over logins that had the password changed. Which one is really newer? (Note previous problem.) We can't store both.

* Logins previously and intentionally deleted from signons.sqlite would be resurrected when re-importing. But logins deleted from signons3.txt won't be removed from signons.sqlite (which is weird for someone expecting a reimport). We could blow away all the "old" signons.sqlite entries, but that carries an enormously high risk of dataloss.

* Risk of problems like bug 462366.

* Triggering any of these problems unknowingly as a result of system clock problems, clock skew between systems (consider Firefox Portable), or someone copying files around and updating the signons3.txt timestamp.

Also, this reimporting issue already exists between Firefox 2 and Firefox 3, but I haven't heard of it being a notable issue... Firefox 2 uses signons2.txt, but Firefox 3 changed the format (bug 396316) and instead uses signons3.txt. Anyone who tried a Firefox 3 pre-release after 5 Dec 2007 had their logins migrated to signons3.txt, and thus further changes to signons2.txt (eg, by switching back FF2) would be ignored.

I don't think this is worth attempting.

Comment 5

[Marien Zwart]

This was indeed not done for bookmarks/history to places.sqlite, but speaking as someone who helped on live chat around the time of the release I would really have liked it if this had been done. It was one of the main (possibly the main, I did not keep statistics) problem there around the time of the release. And http://support.mozilla.com/en-US/kb/Firefox+3+upgrade+switches+bookmarks+and+other+settings+to+older+version is still getting roughly 1300 page views a week (thanks Cww!) (but note that is page *views* for the page with the solution, so does not directly correspond to people with this problem). And that's now, quite a while after the release. Judging from live chat traffic this was a more frequent problem immediately after the release, because the people who used the beta tend to install the final version early too.

So I do not think "We don't do this with other data types (notably, the switch to places.sqlite in FF3)" should matter, although "it's a hairy problem" obviously does. Still, if it is doable to *add* the data from signons in the old format to signons in the new format if the file in the old format is more recently modified I think that would make the upgrade smoother for more people than you might think.

If that turns out to be too complicated to get working reliably (without introducing a way for old data to actually overwrite new data) perhaps a different bug should be opened for improving the chance of people finding out about this *before* they are affected by it, possibly by having this mentioned in the release notes of beta versions or by having it mentioned by the installer or the browser if it detects a profile from a newer version of Firefox (I admit I have not given the latter much thought, but at least mentioning this in the relnotes near the "this upgrade will overwrite Firefox" bit might be reasonable).

And yes, while there is an obvious and reasonably easy (doable by people who apparently installed beta software before) workaround experience with Firefox 3 indicates that a *lot* of people will only notice they have "lost their bookmarks" (or logins), not that they reverted to an older version. Especially for signons I do not think many people know the format changed at all. So "having a page on sumo" is good, but getting people to *find* it is harder than it might seem.

Comment 6

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

(In reply to comment #4)
> * Ambiguity over logins that had the password changed. Which one is really
> newer? (Note previous problem.) We can't store both.

> * Risk of problems like bug 462366.

These two problems can be avoided by ignoring "old" passwords if there are any conflicts during import, and never removing existing passwords.

The other problems you point out boil down to "old passwords might be restored undesirably" (because of a bug with timestamps, or because the user switched back to an older version temporarily, etc.). I think that problem is:

1) less annoying to users than "recent passwords disappear on upgrade", because losing recent passwords on upgrade is expensive, while gaining useless/outdated passwords is cheap.
2) less likely to occur, because removing passwords is uncommon. There aren't many reasons to delete a password apart from wanting to clear your tracks, and users who want to do that are likely to use Clear Private Data, which already deletes old passwords files.

I don't think we need to address the corner case of someone who switches between the two versions every day, and wants the passwords to be in sync. The scenario I care about is someone who tries a 3.1 beta, but then goes back to Firefox 2/3 for a while before eventually installing Firefox 3.1.

Comment 7

[Mike Connor [:mconnor]]

I do not think this should block.  While I accept that this makes life a little harder on beta users, we've never done this for any other format change (signons -> signons2 -> signons3, bookmarks, history, formhistory, downloads, etc).  If there's a straightforward workaround, I don't think this is necessary and we can't do it "right" anyway because of the limitations of the old format.

Yes, it spawns a bunch of questions on release day.  But that's better than having to write and test a bunch of code to do a bit of mitigation (and maybe a bit of damage) to the re-upgrade process.

(In reply to comment #6)
> (In reply to comment #4)
> > * Ambiguity over logins that had the password changed. Which one is really
> > newer? (Note previous problem.) We can't store both.
> 
> > * Risk of problems like bug 462366.
> 
> These two problems can be avoided by ignoring "old" passwords if there are any
> conflicts during import, and never removing existing passwords.

This means:

* Any passwords deleted in 3.1 will be restored from the .txt version. If I have two gmail IDs, and I delete one password so it always autofills, every time we do this I'll go back to the old behaviour.  Lame.
* We'll only net to gaining "new" user/pass combos, but some might be the above set, so this might do more harm than good.

Comment 8

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

(In reply to comment #7)
> Yes, it spawns a bunch of questions on release day.  But that's better than
> having to write and test a bunch of code to do a bit of mitigation (and maybe a
> bit of damage) to the re-upgrade process.

Is it? I have a hard time accepting this at face value, because I think you're underestimating the costs. You and I don't have to deal with the flood of support requests that happen on upgrade, and I think Marien's made a good case that this is something worth considering, even if it's not as likely to be as controversial as bookmarks. That we've dealt with the questions before doesn't mean we should for every scenario where this comes up.

> * Any passwords deleted in 3.1 will be restored from the .txt version. If I
> have two gmail IDs, and I delete one password so it always autofills, every
> time we do this I'll go back to the old behaviour.  Lame.

In the case where you even hit this code, sure. "every time we do this" would be "never" in the common case, and "once" in the "beta user who downgraded once" case. Overall I think it would affect fewer users than the "beta downgrade" scenario (because not all of them will have deleted passwords after downgrading), and would be less likely to actually annoy them, as I said in comment 6.

If you really care about that specific case, we could avoid importing passwords if there's already a password for that host. This would still catch most newly added passwords for users who've downgraded, I think.

I'm not trying to be difficult here, but I think the costs to implementing this would be relatively small, and I think the benefit (reduced user annoyance during upgrade) would be well worth it. Maybe I'm wrong, but it's hard to objectively measure either of the metrics we're basing this decision on (user annoyance and code complexity), so let me at least throw a patch together so we have something concrete to look at?

Comment 9

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

Attachment: WIP

This implements the behavior from the summary - passwords are reimported from signons3.txt if it's lastModifiedTime is greater than signons.sqlite.

It skips logins if a login with the same hostname/realm/formsubmiturl already exists, which means you won't hit the case from comment 7, but also might miss importing some passwords. I reused _queryLogins for simplicitly, but that could obviously be optimized. It also doesn't import exceptions at all (though it could be adjusted to pretty easily).

Minor issues include having to duplicate a bit of code from storage-Legacy (getting the old signons3.txt file), and signons.sqlite won't necessarily be modified after import if none of the passwords were imported, which mean you could potentially keep hitting this code until you either have some new passwords to import from the old file, or trigger a change to the new file somehow (changing/removing/adding a password).

It needs further testing, and there are some rough edges style-wise, but overall this works and isn't *too* complicated, I think. There may be performance implications to the extra stat()s on startup and extra work during import in general, but I'm not sure those are significant.

mconnor/dolske, what do you think? I'll give up fighting a WONTFIX if you're still not convinced, but I think this might have some promise.

Comment 10

[Mark Banner (:standard8)]

I think this is going to cause me really big problems in mailnews. I'm hoping to complete migration to toolkit's password manager fairly soon. As part of this, mailnews will be upgrading some of the old (incorrect) formats it used to store passwords, to the correct formats that follow the current browser implementation (i.e. using up-to-date urls, not from when an account was originally created). This would give us a sane implementation that reflects to the user what is truly happening within the password manager.

To do this, mailnews has no idea of what the password manager is doing. I've currently got two options, add an event into the password manager to tell me its upgrading (something which I think Justin didn't want to do), or have a pref in mailnews to flag upgrade of all the password options.

At least having the signons upgrade at the same time as the switch would mean a fairly sane implementation.

Having a "continous" migration possible like this just throws mailnews into a problem state. I would need to be notified by the password manager of all the logins being migrated, or I would need fallback code to check old-style logins if the new-style wasn't found (and do appropriate migrations).

I'm still working on the migration aspects of mailnews passwords, so its a little hard to say where we'll end up at this stage, but I'm concerned about another layer of complexity here.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> I think this is going to cause me really big problems in mailnews.

I've just been talking to Gavin over irc and remembered this won't be such a problem for mailnews - wallet only ever had signons.txt, so as long as this bug doesn't add reimport functionality from that early (i.e. pre FF 2), then mailnews will be fine.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

FYI, there will be some overlap in code changes between this and bug 451267 (assuming we go forward with this bug). Not that it will effect work on either, but just to keep you informed.

I still hold that while it might not be _that hard_ to implement, it means more code sitting around that we have to maintain - code & tests that have to be added JUST to cover the case of some beta users. That and I still don't think we can effectively cover all of the cases.

Gavin, I agree that it's hard to objectively measure if this is worth it. Maybe we need to look into that more before going further? Or put this off until after b3 gets pushed on the public and see if there's any complaints?

Also, why don't you reimport the disabledHosts? It seems like the same case can be made there as logins.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Paul, I cannot say anything to the underlying code but I will point out that it will not only affect beta users. Also everyone who will use Firefox 3.1 when it is released and realizes that he/she isn't satisfied with some of the new features and will wait until the next or over-next security/stability update. Within this time the old signons3.txt will be used again which gives the same initial situation.

Comment 14

[Tony Mechelynck [:tonymec]]

Yes, and usernames and passwords are a "sensitive" area. I imagine that a case could be written up, where a user (after downgrading from Fx3.1 or later to 3.0 or earlier, changing UN & PW, and later upgrading again) would unknowingly find himself logged in on that site with his "old" username. I hesitate to nominate this bug for "security" status because user actions are necessary, but yet...

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Re-requesting blocking1.9.1 after the bug was getting reopened and is waiting for a final decision with an attached WIP patch.

Justin, can we get an answer from you? Thanks.

Comment 16

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

(In reply to comment #15)
> Re-requesting blocking1.9.1 after the bug was getting reopened and is waiting
> for a final decision with an attached WIP patch.

Still not blocking 1.9.1 - the logic there hasn't changed. Given a safe, tested patch, we'll obviously take the improvement, though.

Comment 17

[Andrew Hagen]

Passwords not showing up is going to just kill sysadmins. 

I am a very knowledgeable FF user, and the lack of this feature took me by surprise. People are used to copying their signons?.txt into their new profile directory.  

If we don't fix it, we need to place information about this change prominently in the release notes.

Comment 18

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

(In reply to comment #17)
> I am a very knowledgeable FF user, and the lack of this feature took me by
> surprise. People are used to copying their signons?.txt into their new profile
> directory.  

It's not clear to me what you mean by "this feature". This bug only causes a problem if you transition back and forth between Firefox 3 and Firefox 3.5, and save passwords in both. signons.sqlite is just as copyable as signons.txt, so nothings changed for the "backup my profile" or "create a new profile but keep my passwords" use cases.

Comment 19

[Andrew Hagen]

By "this feature" I mean the behavior called for in this bug report. 

I had assumed that the password data was still stored in signons.txt. I had no reason to think differently. Signons.txt existed in my profile. Then I wiped the drive and reinstalled the OS and Firefox. When I placed the signons.txt file in the FF profile, the passwords did not register in Password Manager. This was surprising because that was the behavior during my past upgrades and migrations of FF. 

Many users will be highly surprised, and even shocked at this change. Losing data permanently could be an issue for some users. As for me, I was fine, except for the inconvenience, surprise, and even shock that the Password Manager data was gone.  I keep copies of all my passwords elsewhere. 

I did not realize that the Password Manager data is now stored in signons.sqlite, or that signons.sqlite is a portable file. Thank you for that information. 

"Backup my profile" (bug 22689) is not implemented yet. 

To summarize: this is a dataloss bug that needs to be relnoted if we aren't able to fix it in time for 3.5. An import of the data from signons.txt, perhaps after prompting, should really be present in 3.5.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> To summarize: this is a dataloss bug that needs to be relnoted if we aren't
> able to fix it in time for 3.5. An import of the data from signons.txt, perhaps
> after prompting, should really be present in 3.5.
I think you are misunderstanding the situation.  If signons.sqlite does not exist, signons#.txt gets imported.  There is no dataloss unless you are going back and forth between versions which uses signons.sqlite and ones that don't.

This is just like bookmarks not being stored in bookmarks.html anymore.  They get imported once, and that's it.

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

What Shawn said. I know, since I wrote it and we went to great pains to ensure the import process was seamless.

(In reply to comment #19)
> When I placed the signons.txt
> file in the FF profile, the passwords did not register in Password Manager.

You would also need your key3.db file from that other profile so that we can decrypt the passwords. That file keeps track of information used for encryption/decryption. That is most likely the problem you experienced.

Comment 22

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

Attachment: updated to tip

I've kept this in my tree and merged it to tip. Still a WIP, though, and I suspect WONTFIX if no one steps up to work on it.