Closed Bug 1630818 Opened 1 year ago Closed 11 months ago

Update handling of "too new schema" situation now that calendar is integrated into Thunderbird

Categories

(Calendar :: General, task, P3)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1630001 +++

See this comment on bug 1630001: https://bugzilla.mozilla.org/show_bug.cgi?id=1630001#c6

Particularly Geoff's suggestion: "...if we detect a higher database schema version we could do something more useful than refusing to start, like creating a backup of the file as it was and continuing."

An initial patch focused on updating strings: https://phabricator.services.mozilla.com/D71129

Attached patch redo-too-new-schema-0.patch (obsolete) — Splinter Review

This seems like a good opportunity to use the newer OS.File APIs to copy the calendar database file, but I'm getting the error below and the file is not copied.

   {
     fileName: "(unknown module)";
     lineNumber: undefined;
     operation: "open";
     path: "file:///{file-path}/.thunderbird/{profile-dir}/calendar-data/local.sqlite";
     stack: "";
     unixErrno: 2;
   }

When I try to open the file with "DB Browser for SQLite" it tells me "Could not open database file. Reason: database is locked". Maybe that's the problem? My next thought is to try the nsIFile approach used elsewhere in the calendar code.

Attachment #9142560 - Flags: feedback?(geoff)

You have to close the database, presumably in prepareInitDB by catching the exception, calling .close(), then throwing the caught exception.

Attached patch redo-too-new-schema-1.patch (obsolete) — Splinter Review

(In reply to Geoff Lankow (:darktrojan) from comment #2)

You have to close the database, presumably in prepareInitDB by catching the exception, calling .close(), then throwing the caught exception.

Thanks, I tried that along with various other things but kept getting that same error or other ones. After various attempts I had better luck with Services.storage.backupDatabaseFile. That's synchronous which is also simpler in this case since introducing async is viral (callers have to also become async, etc.).

I also moved where we are handling this closer to the source because we now want to just keep carrying on as usual, so it's better to avoid throwing and creating a divergent code path.

The remaining thing to solve is that currently this will copy the file and show the prompt on every startup. So we need a flag to set so we just do this once. A pref would work, but maybe there are better options I'm not thinking of.

Requesting feedback on whether I'm on the right track and on strings, backup file name, etc.

So far I've been testing this by changing the ">" to ">=" on the schema version conditional.

Attachment #9142560 - Attachment is obsolete: true
Attachment #9142560 - Flags: feedback?(geoff)
Attachment #9142916 - Flags: feedback?(geoff)
Status: NEW → ASSIGNED
Comment on attachment 9142916 [details] [diff] [review]
redo-too-new-schema-1.patch

Review of attachment 9142916 [details] [diff] [review]:
-----------------------------------------------------------------

I have to say I'm not sure this is all that useful, at least the alert part. We're not supporting downgrades and I'd imagine users can't really get themselves out of this situation anyway even if that alert is shown. I know it would take more then a little figuring out myself.
Maybe just create the backup file, and put the db version in the filename when you do. You could also log the message to the console.
Attached patch redo-too-new-schema-2.patch (obsolete) — Splinter Review

(In reply to Magnus Melin [:mkmelin] from comment #4)

I have to say I'm not sure this is all that useful, at least the alert part.
We're not supporting downgrades and I'd imagine users can't really get
themselves out of this situation anyway even if that alert is shown. I know
it would take more then a little figuring out myself.
Maybe just create the backup file, and put the db version in the filename
when you do. You could also log the message to the console.

Makes sense to me. I've taken this approach with this patch. As Geoff and I discussed on chat, if there is already a copy of the file, we don't make a new one or log the error.

Attachment #9142916 - Attachment is obsolete: true
Attachment #9142916 - Flags: feedback?(geoff)
Attachment #9143124 - Flags: review?(geoff)

Instead of copying, should we just rename the existing file and start a new one? Then we would not be trying to use some scheme that has changed in ways we can't possibly know at this point.

For the user it would appear that all of their events are gone, to which we say "we told you not to downgrade, here's where to find a backup that we helpfully created". If we did it this way we should create a backup every time, with the date in the file name.

That seems like a reasonable way to handle it. How is downgrading handled elsewhere for Thunderbird? Do users get some kind of message at some point that they are heading into the wilderness? I'd feel better about having their events disappear if they've gotten that message in some clear way first. This seems like an area where we want to be careful with the UX.

I'm going to wait on a final decision on what should happen here before implementing it. (This is the kind of thing I had in mind with my question about UI/UX design process in the Thunderbird team meeting yesterday. Having a clear idea of the UI/UX design helps avoid unnecessary cycles on the implementation.)

Flags: needinfo?(mkmelin+mozilla)

There's not any downgrade handling elsewhere. Data formats haven't moved a lot, but when they do it's a one way street. The driving force for profile-per-install that firefox (and we) implemented for 68 was that you can't really go back and expect all data to be there and correct. If we do a backup, might also be better to do an export to a .ics file so there's a reasonable way to import it later if needed. (Or do we do those backups already, lightning used to at some point.)

Flags: needinfo?(mkmelin+mozilla)

If I'm properly understanding the situation, I think we should extend the suggestion introduced by Magnus.
Since we don't support a clear downgrade path, and if I'm not wrong we're not planning to support it at all, we should import everything that can be imported into the new profile.

IMHO, the optimal solution would be to:

  • Create a backup and init a new schema to not interrupt the launch.
  • Warn the user that we did it for all the reasons listed above.
  • Ask the user if they want to try to import the old data from the backup.
  • Try to import what's importable from the old backup (calendars, signatures, accounts, etc)

I don't know how feasible this is, but I think it's important to offer some sort of "restore" process if we can manage to extract data that can be imported from the old backup.

Apologies if I completely missed the point of this bug/discussion.

For upgrades the profile is kept, and it must be. It can be tens of GB.
We warn (and basically disallow) downgrades - you can't start an old profile unless you use --allow-downgrade

The restore process would probably be best to do by using the newer profile again, and from there export the data. Then in the old profile re-import it. It's all a pain of course, but it is what it is. OR, restore from an outside backup.

Attached image downgrade-dialog.png

Okay, so the user gets fair warning that they're doing something unsupported because they get the attached alert when opening an older version of TB with a newer profile.

<sidenote>
Clicking on the "More information..." link, I got a not very helpful dialog like this:

Title: Close Firefox
Message: Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system.

</sidenote>

If I understand it correctly, (and I think this is just re-stating what Magnus said in more detail) the best way to downgrade calendar data is to

  • first export any local calendars to .ics files before downgrading
  • write down all calendar setup info like urls, logins, passwords for networked calendars, etc.
  • downgrade: open the older TB with the profile using --allow-downgrade
  • set up your calendars again, importing .ics files for local calendars, and re-subscribing to networked calendars

So Geoff's suggestion in comment 6 is the way to go because it would be much better to import into a fresh calendar data file, and then we know the data file has the right schema for the version of TB.

Making a backup copy of the data file is just an extra fallback so the data is not lost. That also allows immediately switching back to the newer Thunderbird. If the user immediately switches back to the newer version of Thunderbird (maybe they realized they needed to export their calendar data when their events were all gone...) their events will still be gone, but they can then rename the backup file to get them back and then export them.

Attached patch redo-too-new-schema-3.patch (obsolete) — Splinter Review

(In reply to Geoff Lankow (:darktrojan) from comment #6)

Instead of copying, should we just rename the existing file and start a new one? Then we would not be trying to use some scheme that has changed in ways we can't possibly know at this point.

For the user it would appear that all of their events are gone, to which we say "we told you not to downgrade, here's where to find a backup that we helpfully created". If we did it this way we should create a backup every time, with the date in the file name.

This patch takes this approach (which mkmelin said made sense via chat). The implementation is a bit awkward because it involves some back and forth between calStorageUpgrade.jsm and CalStorageCalendar.jsm.

I did some manual testing to check that this works. A copy of the "local.sqlite" file with the date stamp is indeed created by the "too new schema" code path, and a new empty "local.sqlite" file is created and it works for storing new events.

Something I didn't expect: running TB with the "too new schema" code getting called, the previously created events were not gone, but were still visible. Opening TB again after that (without the "too new schema" code getting called) and they were gone. So it appears that events are loaded into memory before the database upgrade happens? Maybe that's fine since this whole scenario is unsupported, or maybe there's an easy fix for it.

Attachment #9143124 - Attachment is obsolete: true
Attachment #9143124 - Flags: review?(geoff)
Attachment #9148220 - Flags: review?(geoff)
Comment on attachment 9148220 [details] [diff] [review]
redo-too-new-schema-3.patch

R+, but you really really must call `storageCalendar.db.close();` after the rename operation. Otherwise you're still using the old database (as happened to you) or there's infinite recursion death (as happened to me).
Attachment #9148220 - Flags: review?(geoff) → review+

(In reply to Geoff Lankow (:darktrojan) from comment #13)

Comment on attachment 9148220 [details] [diff] [review]
redo-too-new-schema-3.patch

R+, but you really really must call storageCalendar.db.close(); after the
rename operation. Otherwise you're still using the old database (as happened
to you) or there's infinite recursion death (as happened to me).

Aha! That makes sense. Thanks, and thanks for the review. I've added that line to close the database after renaming the file. (Apologies for the infinite recursion death.)

Attachment #9148220 - Attachment is obsolete: true
Attachment #9150255 - Flags: review+

.

The try run seems okay, from what I can tell. I just pinned a number of bugs for the test failures, they all seem like known intermittent failures.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/15f54240ef82
Rework handling of "too new calendar schema" error. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 78
You need to log in before you can comment on or make changes to this bug.