530079 - malformed URI sequence when migrating passwords from TB 2 to TB 3

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Gyorgy Szilagyi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: 20091112084939

When upgrading from TB2.0.0.23 to TB3 RC1 (English GB locale,
running on English Windows Vista), the signons.sqlite file (which should store the passwords for the various accounts) is not created.

In TB2 all the passwords were saved, but after the upgrade to TB3 RC1 the password manager is empty. When trying to download or send e-mail, TB would try to retrieve the saved password but fail ("Could not get password"), yet it would not prompt for the password to be entered manually, i.e. it is impossible to send/receive messages both for IMAP and POP accounts.

New profiles created in TB3 RC1 will include the signons.sqlite file. Copying this file to the "old" profile will "force" TB to prompt for passwords.

Reproducible: Didn't try

Steps to Reproduce:
1. Install TB3 RC1 on top of TB2.0.0.23
2.
3.
Actual Results:  
Can't send or receive mail, saved passwords lost yet TB does not even prompt for them to be entered manually.

Expected Results:  
Continue to be able to send/receive mails using saved passwords after the upgrade.

See attached the error console (with signons.debug ON).

Comment 1

[Gyorgy Szilagyi]

Attachment: Error console with signons.debug on

Comment 2

[Justin Dolske [:Dolske]]

This was spun out from bug 524623. My comment there:

Specifically, the "malformed URI sequence" implies the reading of the legacy
wallet format is out of sync (see bug 474846). Either the import code needs to
wrap some newURI calls with try/catch, or the impl. for those URIs needs to not
throw.

Comment 3

[Ludovic Hirlimann [:Usul]]

Ok I just did a simple test . and the file got created. I updated to rc1 build3. But I only had one pop3 account.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I definitely had broken important parts of the SQLite code for the password manager for RC 1 build 2...

Comment 5

[Mark Banner (:standard8)]

Gyorgy, what is the format of your account username and/or hostname? Do they just contain letters (a-z) or other characters as well?

Comment 6

[Mark Banner (:standard8)]

I'm confirming this bug based on what we've seen here, in get satisfaction, and possibly bug 524623 comment 19 (although I don't fully understand what's happening in that comment).

I think from the reports there is an issue somewhere between this bug and bug 524623.

I've never had anyone give me an enough info to reproduce, and with what I was given there was never anything obviously different or wrong.

However, I think the best option is to add some protection in storage-Legacy.js so that the problem entries are ignored, we migrate what we can without error, and allow TB to continue functioning wrt passwords.

Comment 7

[Mark Banner (:standard8)]

Attachment: Proposed fix / work around

As seen in the console log attached to this bug, it is the decodeURIComponent(username) line that is failing.

Therefore, do two things: 1) catch and log the specific failure, including the username; 2) catch any errors translating an entry and not add the entry to the new database.

This second step allows the database creation and therefore password management to continue, and hence even if some password data is lost, the user can either go back to the previous version to find it, or re-enter the information. Either way, they will be able to continue using Thunderbird.

I've also added a broken version that I made up into the tests, and that is handled correctly.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 444077 [details] [diff] [review]
Proposed fix / work around

There's a bunch of whitespace changes in this patch, could you resubmit without those? General approach looks fine otherwise, will review in detail without whitespace changes.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 444077 [details] [diff] [review])
> There's a bunch of whitespace changes in this patch, could you resubmit without
> those? General approach looks fine otherwise, will review in detail without
> whitespace changes.

Sorry, that's my emacs automatically cleaning up whitespace at end of lines. I'll have to hack that out in the patch, but I can attach a new version.

Comment 10

[Justin Dolske [:Dolske]]

Ah. I'd also take a patch (in a separate bug) to just kill dumb whitespace, if that makes things easier.

Comment 11

[Mark Banner (:standard8)]

Attachment: Proposed fix / work around v2

Without the whitespace changes.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

It would be nice to have to know exactly what's failing, but this seems like a safe bandaid.

>+                try {
>+                    username = decodeURIComponent(username);
>+                }
>+                catch (ex) {

Nit: Braces and catch on same line:

  } catch (ex) {

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> (From update of attachment 444855 [details] [diff] [review])
> It would be nice to have to know exactly what's failing, but this seems like a
> safe bandaid.

I quite agree. Unfortunately, all the cases I've given appear to be "valid" cases. My only thought is that sometimes the 2.x corrupted the username entry or something, but that's difficult to tell without a case.

Anyway, checked in:

http://hg.mozilla.org/mozilla-central/rev/95afe406d8d5

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

Requesting approval for 1.9.2 branch:

Thunderbird needs this to provide a band-aid around password upgrading (when going from 2.x to 3.x) for some instances where something causes failure and then stops passwords being remembered.

The patch adds a couple of try/catch statements to handle the errors, and includes a test case.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #14)
> (From update of attachment 444855 [details] [diff] [review])
> Requesting approval for 1.9.2 branch:

How about 1.9.1? I guess TB may be fine with having a fix for 3.1 only since it's not too far off but SM won't have a release for several months and is affected just as well.

Comment 16

[Robert Kaiser]

Usually, we get things onto branches in order, i.e. "first 1.9.2, then 1.9.1", in this case. It might still make sense to just request for both at the same time, though. I'd surely like to have it on 1.9.1.

Comment 17

[LegNeato]

Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.

Code freeze is on 2010-06-25.

If this needs to be nominated for 1.9.1 feel free to do so.

Comment 18

[Mark Banner (:standard8)]

Checked into mozilla-1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a58c254dee96

Comment 19

[LegNeato]

Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

a=LegNeato for 1.9.1.11 as well

Comment 20

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03998dbfd6ea

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Since there are no STR, there is nothing for QA to do to verify this fix for 1.9.1 or 1.9.2.