Closed
Bug 530079
Opened 15 years ago
Closed 14 years ago
malformed URI sequence when migrating passwords from TB 2 to TB 3
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: gyszilagyi, Assigned: standard8)
References
Details
(Whiteboard: [223 Migration][tb31needed] [qa-ntd-191] [qa-ntd-192])
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Dolske
:
review+
christian
:
approval1.9.2.7+
christian
:
approval1.9.1.11+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Comment 1•15 years ago
|
||
Comment 2•15 years ago
|
||
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.
Updated•15 years ago
|
Comment 3•15 years ago
|
||
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•15 years ago
|
||
I definitely had broken important parts of the SQLite code for the password manager for RC 1 build 2...
Assignee | ||
Comment 5•15 years ago
|
||
Gyorgy, what is the format of your account username and/or hostname? Do they just contain letters (a-z) or other characters as well?
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee: nobody → bugzilla
Blocks: 524623
Status: UNCONFIRMED → NEW
Component: Migration → Password Manager
No longer depends on: 524623
Ever confirmed: true
Product: Thunderbird → Toolkit
QA Contact: migration → password.manager
Hardware: x86 → All
Summary: TB 2.0.0.23 -> 3.0 RC1 missing signons.sqlite file → malformed URI sequence when migrating passwords from TB 2 to TB 3
Assignee | ||
Comment 7•14 years ago
|
||
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.
Attachment #444077 -
Flags: review?(dolske)
Updated•14 years ago
|
Attachment #444077 -
Flags: review?(dolske)
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
Ah. I'd also take a patch (in a separate bug) to just kill dumb whitespace, if that makes things easier.
Assignee | ||
Comment 11•14 years ago
|
||
Without the whitespace changes.
Attachment #444077 -
Attachment is obsolete: true
Attachment #444855 -
Flags: review?(dolske)
Comment 12•14 years ago
|
||
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) {
Attachment #444855 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 13•14 years ago
|
||
(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
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Comment 14•14 years ago
|
||
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.
Attachment #444855 -
Flags: approval1.9.2.5?
Assignee | ||
Updated•14 years ago
|
Whiteboard: [223 Migration] → [223 Migration][tb31needs]
Comment 15•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [223 Migration][tb31needs] → [223 Migration][tb31needs][rc1/final fixed][needs approval 1.9.2.5 for TB 3.1.1]
Comment 16•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #444855 -
Flags: approval1.9.2.5? → approval1.9.2.6+
Updated•14 years ago
|
Attachment #444855 -
Flags: approval1.9.1.11?
Assignee | ||
Comment 18•14 years ago
|
||
Checked into mozilla-1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a58c254dee96
status1.9.2:
--- → .6-fixed
Whiteboard: [223 Migration][tb31needs][rc1/final fixed][needs approval 1.9.2.5 for TB 3.1.1] → [223 Migration][tb31needed]
Comment 19•14 years ago
|
||
Comment on attachment 444855 [details] [diff] [review]
Proposed fix / work around v2
a=LegNeato for 1.9.1.11 as well
Attachment #444855 -
Flags: approval1.9.1.11? → approval1.9.1.11+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 20•14 years ago
|
||
status1.9.1:
--- → .11-fixed
Updated•14 years ago
|
Keywords: checkin-needed
Comment 21•14 years ago
|
||
Since there are no STR, there is nothing for QA to do to verify this fix for 1.9.1 or 1.9.2.
Whiteboard: [223 Migration][tb31needed] → [223 Migration][tb31needed] [qa-ntd-191] [qa-ntd-192]
You need to log in
before you can comment on or make changes to this bug.
Description
•