Closed Bug 472064 Opened 11 years ago Closed 11 years ago

Satchel needs better upgrade/downgrade logic

Categories

(Toolkit :: Form Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: verified1.9.0.7, verified1.9.0.9, verified1.9.1)

Attachments

(2 files, 4 obsolete files)

Over in bug 467463, I made the password manager's signons.sqlite properly deal with schema upgrades and downgrades (including the fun case of upgrading from v1 to v2, downgrading to v1, and then reupgrading to v2).

Satchel needs this logic too. The timesUsed, firstUsed, and lastUsed columns should also not be created with "NOT NULL", so that an older satchel can still add rows and we can add the timestamps when reupgrading.
Flags: blocking1.9.1?
To fix the DB manually (to use a current DB from FF3.1/3.2 on FF3.0), the following should work:

$ sqlite3 formhistory.sqlite 
SQLite version 3.4.0
Enter ".help" for instructions
sqlite> BEGIN TRANSACTION;
sqlite> CREATE TEMPORARY TABLE oops_table (id INTEGER PRIMARY KEY, fieldname TEXT NOT NULL, value TEXT NOT NULL);
sqlite> INSERT INTO oops_table SELECT id, fieldname, value FROM moz_formhistory;
sqlite> DROP TABLE moz_formhistory;
sqlite> CREATE TABLE moz_formhistory (id INTEGER PRIMARY KEY, fieldname TEXT NOT NULL, value TEXT NOT NULL);
sqlite> INSERT INTO moz_formhistory SELECT id, fieldname, value FROM oops_table;
sqlite> DROP TABLE oops_table;
sqlite> PRAGMA user_version = 0;
sqlite> COMMIT;

Keep a backup of the original formhistory.sqlite, lest something blow up. :)

The SQLite Manager extension can also do this, either by entering the above SQL or using the "Drop Column" buttons on the timesUsed, firstUsed, and lastUsed columns and setting the "User Version" to 0.
Attached patch Patch v.1 (obsolete) — Splinter Review
After implementing this, I realized we should probably add a schema version bump for correcting the column definitions. (IE, a v.2 that basically does what's in comment 1, except instead of removing the columns it re-adds them without the "NOT NULL" conditions).

This might have some short-term benefit for people flipping between recent builds, but I'm mainly thinking about the long-term prospects of having a bunch of people who have "NOT NULL" columns defs... Say, if a year or three down the road we want to deliberately store a null value in one of these fields for some reason.

OTOH this is kind of an edge-case, but it seems like the right thing to do.
The steps from comment 1 are fine. Thanks Justin. Will we also get a test for that?
Flags: in-testsuite?
Yes, which is part of the reason why this patch isn't finished yet. I'm quite capable of driving my own bugs, please stop trying to micromanage.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
After further thought, I now take back comment 2.

The "NOT NULL" condition was only set on databases created for the first time with the code from bug 463154. EG, people creating a new profile with 3.1 Beta 2 (or a 3.1/3.2 nightly since then). Columns added when upgrading an existing formhistory.sqlite are *not* created with the "NOT NULL" condition.

So, FF3 users who try out a FF3.1 beta are not affected by this problem. That's the case I was most worried about. I should think there are only a tiny number of users who might have created a profile with a beta, and then tried to use that profile with an older version of Firefox.

Given the small number of people this potentially affects, that formhistory isn't critical data, and there's a manual workaround (comment 1), I'm inclined not to add code to handle this rare case. Altering the existing DB schema is fairly invasive, and could have performance issues, so subjecting all users to this isn't worth it.

I'll be adding tests to the patch already attached, and we'll go with that.

[Note that this does fix up the initial DB creation, so that in the future users who create a profile with a 3.1B3+ build will be able to use it on 3.0.]
Attached patch Patch v.2 (obsolete) — Splinter Review
Updated patch w/tests.

Found an amusing bug, too... The PR_HOURS #define was overflowing, because 86 billion won't quite fit into an |int|. Noticed this when adding tests to verify that a correct timestamp was being added. [Which confused the heck out of me -- the resulting 32 bit value exactly 500.6 seconds, so I was thinking something was explicitly adjusting the time by such a round value. :-)]
Attachment #355498 - Attachment is obsolete: true
Attachment #357092 - Flags: review?(gavin.sharp)
Attachment #357092 - Flags: review?(sdwilsh)
Comment on attachment 357092 [details] [diff] [review]
Patch v.2

r=sdwilsh
Attachment #357092 - Flags: review?(sdwilsh) → review+
Whiteboard: [needs review gavin]
Comment on attachment 357092 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/satchel/src/nsStorageFormHistory.cpp b/toolkit/components/satchel/src/nsStorageFormHistory.cpp

>+  // Ensure DB is at the current schema.
>+  dbMigrate();
>   NS_ENSURE_SUCCESS(rv, rv);

Looks like an assignment to rv missing here.

>diff --git a/toolkit/components/satchel/test/unit/test_db_update_v999b.js b/toolkit/components/satchel/test/unit/test_db_update_v999b.js

>+  // First init should fail, second init should succeed.

talked to dolske on IRC, I think we should make init never fail, and he's working on a patch that does that I believe.
Attachment #357092 - Flags: review?(gavin.sharp)
Attached patch Patch v.3 (obsolete) — Splinter Review
Fixed previous comments, and added some extra tests to make sure the corrupt DB backup file is created when expected.
Attachment #357092 - Attachment is obsolete: true
Attachment #358114 - Flags: review?(gavin.sharp)
Comment on attachment 358114 [details] [diff] [review]
Patch v.3

>diff --git a/toolkit/components/satchel/src/nsStorageFormHistory.cpp b/toolkit/components/satchel/src/nsStorageFormHistory.cpp

> nsFormHistory::Init()

>-  PRBool doImport = PR_FALSE;
>+  PRBool doImport;

>   nsresult rv = OpenDatabase(&doImport);
>+  if (rv == NS_ERROR_FILE_CORRUPTED) {
>+    /* If the DB is corrupt, nuke it and try again with a new DB. */
>+    dbCleanup();

Should probably rv check dbCleanup().

>+    rv = OpenDatabase(&doImport);
>+    doImport = PR_FALSE;

Shouldn't be necessary, right? We only use this if OpenDatabase succeeded, in which case we should be sure it's set. Could just leave the default value if you want to be careful.

> nsFormHistory::CreateTable()

>-           "id INTEGER PRIMARY KEY, fieldname TEXT NOT NULL, "
>+           "id INTEGER PRIMARY KEY, fieldname TEXT NULL, "

dolske says on IRC that this is a mistake.

>+nsFormHistory::MigrateToVersion1()

>+  PRBool columnsExist = PR_FALSE;
>+  if (NS_SUCCEEDED(rv))
>+    columnsExist = PR_TRUE;

assign directly?

>+nsFormHistory::dbCleanup()

>+  nsCOMPtr<mozIStorageService> storage =
>+    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(storage, NS_ERROR_NOT_AVAILABLE);

Could use mStorageService (dbCleanup is only ever called after OpenDatabase, and not if mStorageService's initialization failed - wouldn't hurt to ASSERT though).

r=me with those fixed, or ignored (but not the mistake)!
Attachment #358114 - Flags: review?(gavin.sharp) → review+
Whiteboard: [needs review gavin] → [has patch]
(In reply to comment #10)
> wouldn't hurt to ASSERT though

er, but I guess you'd crash if it did, so no point asserting. perhaps just disregard that entire comment? doesn't really matter.
(In reply to comment #10)

> >+    rv = OpenDatabase(&doImport);
> >+    doImport = PR_FALSE;
> 
> Shouldn't be necessary, right? We only use this if OpenDatabase succeeded, in
> which case we should be sure it's set. Could just leave the default value if
> you want to be careful.

doImport will be always be true after this OpenDatabase call, because we just nuked whatever (corrupt) DB existed, and created a new DB. I think we don't want to try importing from formhistory.dat, because if it exists it's most likely going to be ancient FF2 data. [In my daily use profile, I still have a formhistory.dat that was last modified in Fall 2007.]
Whiteboard: [has patch] → [needs landing]
Attached patch Patch v.4Splinter Review
Nits addressed. Will checkin tomorrow.
Attachment #358114 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/bd6e3311bfab
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Attached patch Patch for FF3.0, v.1 (WIP) (obsolete) — Splinter Review
We should really fix the schema checking on branch too, so that people flipping between Firefox 3.0 and Firefox 3.1 don't risk wonkyness.

To reduce risk, only a tiny subset of the trunk patch is needed. I don't expect we'll be changing the schema version in a Firefox 3.0.x release, so all it really needs is to downgrade the schema version if it encounters a DB with a newer schema. Dunno if it's worth having it handle an incompatible future version, since in principle we shouldn't be doing that anyway. Will look at that tomorrow, along with backporting some tests.
Flags: blocking1.9.0.7?
Same as last the last (WIP v.1) patch, but with tests copied over from mozilla-central.
Attachment #360283 - Attachment is obsolete: true
Attachment #360457 - Flags: review?(gavin.sharp)
Attachment #360457 - Flags: review?(gavin.sharp) → review+
Justin: Is this patch ready for 1.9.0? If so, can you request approval1.9.0.7 on it? We'll look at it later this afternoon.
Comment on attachment 360457 [details] [diff] [review]
Patch for FF3.0, v.2

This is a low-risk change. For most users this will be a no-op, it only does something if you're using a profile that has been used by a newer version of Firefox.
Attachment #360457 - Flags: approval1.9.0.7?
Comment on attachment 360457 [details] [diff] [review]
Patch for FF3.0, v.2

Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #360457 - Flags: approval1.9.0.7? → approval1.9.0.7+
Flags: blocking1.9.0.7? → blocking1.9.0.7+
Checked in on branch:

Checking in toolkit/components/satchel/src/nsStorageFormHistory.cpp;
  new revision: 1.23; previous revision: 1.22
Checking in toolkit/components/satchel/test/unit/formhistory_apitest.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_v999a.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/head_satchel.js;
  new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/satchel/test/unit/test_db_update_v999a.js;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/test_history_api.js;
  initial revision: 1.1
Keywords: fixed1.9.0
Verified for 1.9.0.7 using unit tests, which are passing, since there is no other direct way to test this for QA.
Verified for 1.9.1 using unit tests as well.
Blocks: 483096
Al, as what I can see it has not been fixed for Firefox 3.0.7. See bug 463154 comment 8.
Henrik: See comment 5.
Thanks Justin. I'll point affected users to this bug and will mention the workaround so they will not loose their entered form data.
The full patch for this has landed on 1.9.0.8 as part of bug 483096. The previous patch to 1.9.0.7 was just a minimized version; other changes have required taking the full patch as landed on 1.9.1 / 1.9.2.
Keywords: fixed1.9.0.8
Verified for 1.9.0.8 using bug 483096 and its tests.
Duplicate of this bug: 468642
Duplicate of this bug: 480959
Duplicate of this bug: 481810
You need to log in before you can comment on or make changes to this bug.