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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Password Manager
--
major
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Gyorgy Szilagyi, Assigned: standard8)

Tracking

unspecified
mozilla1.9.3a5
All
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .7-fixed, status1.9.1 .11-fixed)

Details

(Whiteboard: [223 Migration][tb31needed] [qa-ntd-191] [qa-ntd-192])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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

8 years ago
Created attachment 413613 [details]
Error console with signons.debug on
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

8 years ago
Blocks: 524434
Depends on: 524623
Whiteboard: [223 Migration]
Ok I just did a simple test . and the file got created. I updated to rc1 build3. But I only had one pop3 account.
I definitely had broken important parts of the SQLite code for the password manager for RC 1 build 2...
(Assignee)

Comment 5

7 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

7 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

7 years ago
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.
Attachment #444077 - Flags: review?(dolske)
Attachment #444077 - Flags: review?(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.
(Assignee)

Comment 9

7 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.
Ah. I'd also take a patch (in a separate bug) to just kill dumb whitespace, if that makes things easier.
(Assignee)

Comment 11

7 years ago
Created attachment 444855 [details] [diff] [review]
Proposed fix / work around v2

Without the whitespace changes.
Attachment #444077 - Attachment is obsolete: true
Attachment #444855 - Flags: review?(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) {
Attachment #444855 - Flags: review?(dolske) → review+
(Assignee)

Comment 13

7 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
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 14

7 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

7 years ago
Whiteboard: [223 Migration] → [223 Migration][tb31needs]
(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

7 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

7 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

7 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+
Attachment #444855 - Flags: approval1.9.1.11?
(Assignee)

Comment 18

7 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

7 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

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03998dbfd6ea
status1.9.1: --- → .11-fixed

Updated

7 years ago
Keywords: checkin-needed
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.