Last Comment Bug 530079 - malformed URI sequence when migrating passwords from TB 2 to TB 3
: malformed URI sequence when migrating passwords from TB 2 to TB 3
Status: RESOLVED FIXED
[223 Migration][tb31needed] [qa-ntd-1...
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: unspecified
: All Windows Vista
: -- major with 1 vote (vote)
: mozilla1.9.3a5
Assigned To: Mark Banner (:standard8)
:
Mentors:
Depends on:
Blocks: qa-tb3.0rc1 524623
  Show dependency treegraph
 
Reported: 2009-11-20 07:17 PST by Gyorgy Szilagyi
Modified: 2010-07-15 16:28 PDT (History)
13 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.7-fixed
.11-fixed


Attachments
Error console with signons.debug on (322.96 KB, image/png)
2009-11-20 07:19 PST, Gyorgy Szilagyi
no flags Details
Proposed fix / work around (7.75 KB, patch)
2010-05-07 06:57 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
Proposed fix / work around v2 (4.24 KB, patch)
2010-05-12 03:46 PDT, Mark Banner (:standard8)
dolske: review+
christian: approval1.9.2.7+
christian: approval1.9.1.11+
Details | Diff | Review

Description Gyorgy Szilagyi 2009-11-20 07:17:48 PST
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 2009-11-20 07:19:52 PST
Created attachment 413613 [details]
Error console with signons.debug on
Comment 2 Justin Dolske [:Dolske] 2009-11-20 15:44:32 PST
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] 2009-11-23 07:10:33 PST
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] 2009-11-23 08:07:20 PST
I definitely had broken important parts of the SQLite code for the password manager for RC 1 build 2...
Comment 5 Mark Banner (:standard8) 2009-12-18 12:14:27 PST
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) 2010-05-07 06:33:40 PDT
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) 2010-05-07 06:57:04 PDT
Created attachment 444077 [details] [diff] [review]
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] 2010-05-10 14:09:27 PDT
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) 2010-05-10 14:40:25 PDT
(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] 2010-05-10 17:16:31 PDT
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) 2010-05-12 03:46:34 PDT
Created attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

Without the whitespace changes.
Comment 12 Justin Dolske [:Dolske] 2010-05-13 19:47:10 PDT
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) 2010-05-14 02:48:02 PDT
(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) 2010-05-14 02:49:46 PDT
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) 2010-05-14 09:23:36 PDT
(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 (not working on stability any more) 2010-05-15 05:59:10 PDT
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 christian 2010-06-21 17:34:34 PDT
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) 2010-06-22 03:58:04 PDT
Checked into mozilla-1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a58c254dee96
Comment 19 christian 2010-06-24 13:54:38 PDT
Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

a=LegNeato for 1.9.1.11 as well
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2010-06-25 14:40:52 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03998dbfd6ea
Comment 21 Al Billings [:abillings] 2010-07-15 16:28:01 PDT
Since there are no STR, there is nothing for QA to do to verify this fix for 1.9.1 or 1.9.2.

Note You need to log in before you can comment on or make changes to this bug.