470430 - Upgrade 0.3.1 to 0.9/1.0pre fails [Error: Upgrade failed! DB Error: duplicate column name: is_organizer]

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081219 Calendar/1.0pre (BuildID: 20081219033507)

Steps To Reproduce:
===================

- create new profile using Sunbird 0.3.1
- create sample event and task
- close Sunbird
- open profile using Sunbird 1.0pre

Actual results:
===============

Console output:
---------------

**** Upgrading schema from 5 -> 6
**** Upgrading schema from 6/7/8/9 -> 10
Error: Upgrade failed! DB Error: duplicate column name: is_organizer
Error: Upgrade failed! DB Error: duplicate column name: is_organizer

Two error popup dialogs:
------------------------

An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///E:/sunbird/calendar-js/calStorageCalendar.js :: addColumn :: line 899"  data: no]

Error Console output:
---------------------

Error: Assert failed: An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.
2: [file:///E:/sunbird/calendar-js/calCalendarManager.js:560] cmgr_createCalendar
3: [file:///E:/sunbird/calendar-js/calCalendarManager.js:710] cmgr_assureCache
4: [file:///E:/sunbird/calendar-js/calCalendarManager.js:686] cmgr_getCalendars
5: [null:0] null
6: [file:///E:/sunbird/components/calCompositeCalendar.js:178] anonymous
7: [null:0] null
8: [chrome://calendar/content/calUtils.js:1874] getCompositeCalendar
9: [chrome://calendar/content/calendar-task-tree.xml:806] 
Source File: file:///E:/sunbird/calendar-js/calUtils.js
Line: 956

Error: Assert failed: An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.
2: [file:///E:/sunbird/calendar-js/calCalendarManager.js:560] cmgr_createCalendar
3: [null:0] null
4: [chrome://calendar/content/calendar-management.js:94] loadCalendarManager
5: [chrome://calendar/content/calendar-chrome-startup.js:43] commonInitCalendar
6: [chrome://calendar/content/calendar.js:52] calendarInit
7: [chrome://sunbird/content/calendar.xul:1] onload

Source File: file:///E:/sunbird/calendar-js/calUtils.js
Line: 956

Comment 1

[Tom]

I have a very similar problem. I recently upgraded from Windows English Sunbird 0.3.1 to .9. After updating I received the following in the error console:

Error: Upgrade failed! DB Error: duplicate column name: is_organizer
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendarModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calStorageCalendar.js :: addColumn :: line 898"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendarModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calStorageCalendar.js
Line: 898
Error: Assert failed: An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.
1: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calUtils.js:1004] ASSERT
2: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js:560] cmgr_createCalendar
3: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js:708] cmgr_assureCache
4: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js:684] cmgr_getCalendars
5: [null:0] null

Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calUtils.js
Line: 1009
Error: Upgrade failed! DB Error: duplicate column name: is_organizer
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendarModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calStorageCalendar.js :: addColumn :: line 898"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendarModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calStorageCalendar.js
Line: 898
Error: Assert failed: An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.
1: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calUtils.js:1004] ASSERT
2: [file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js:560] cmgr_createCalendar
3: [null:0] null
4: [chrome://calendar/content/calendar-management.js:114] loadCalendarManager
5: [chrome://calendar/content/calendar-chrome-startup.js:43] commonInitCalendar

Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calUtils.js
Line: 1009
Error: calendar has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js
Line: 580
Error: uncaught exception: [Exception... "'[JavaScript Error: "calendar has no properties" {file: "file:///C:/Program%20Files/Mozilla%20Sunbird/components/calItemModule.js -> file:///C:/Program%20Files/Mozilla%20Sunbird/js/calCalendarManager.js" line: 580}]' when calling method: [calICalendarManager::registerCalendar]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-management.js :: loadCalendarManager :: line 116"  data: yes]
Error: calendar has no properties
Source File: chrome://calendar/content/calendar-task-editing.js
Line: 107

It appears to be linked to retrieving events.

Comment 2

[Stefan Sitter [:ssitter]]

Either database upgrade should work or should be forbidden. If upgrade from old version is not possible there should be a user notice to upgrade e.g. to 0.7 first before upgrading to 0.9.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Last time I looked at all the upgrade code, I though it was just a matter of time that our upgrade code would bork on old versions, since we assume that sqlTables contains a version that fits with the current upgrade, but it actually contains the newest version only.

I think to correctly solve this problem and clean out all the rot we should identify which database versions match to which releases and refactor all the update code to cumulatively update from the database version that fits with 0.3 to the database that fits with 0.3.1 and the same for 0.3.1 -> 0.5 and so on.

This way we have cleaned out all the intermediate updates in developer versions. We also need to rewrite all update code that relies on sqlTables. To test this, it would be great to have a database from each release version that contains the same set of events (i.e imported from an ics file, not as a result of our current upgrade!!). Then we could take the 0.2 database, upgrade it to 0.3 and compare with the reference database.

This all sounds like a lot of work (and it is!), but I think every other solution would be quite a botch job.

Stefan, do you think you could provide us with a canned ics file that fits with all aspects of our upgrade process, and a set of databases created by importing the ics file into each release version? If possible, also find out which database version fits to which release.

From here we can see how to continue.

Comment 7

[Darlene Ford]

Same bug also occurs when upgrading profile from unknown earlier release to Sunbird 0.9.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

Work in progress. Totally untested, I probably won't have time to continue on this bug in the next couple of weeks.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

This needs to happen for 1.0 in some form. Better yet it needs to happen for 1.0b1, since upgrading a 0.9 profile fails with bug 494140 applied (which blocks the beta)

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 387689 [details] [diff] [review]
Fix - v1

After some testing it seems this patch works fine the way it is, please review.

Comment 12

[Stefan Sitter [:ssitter]]

Comment on attachment 387689 [details] [diff] [review]
Fix - v1

Could you provide an updated patch that applies to the current source?

$ hg import --no-commit ../470430-storage-upgrade.diff
applying ../470430-storage-upgrade.diff
patching file calendar/providers/storage/calStorageCalendar.js
Hunk #3 FAILED at 693
Hunk #7 FAILED at 2098
2 out of 10 hunks FAILED -- saving rejects to file calendar/providers/storage/calStorageCalendar.js.rej
abort: patch failed to apply

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

This should do it.

Comment 14

[Stefan Sitter [:ssitter]]

Comment on attachment 393875 [details] [diff] [review]
Fix - v2

First check: Creating a profile fails because the new files are not in place:

Error: Error opening input stream (invalid filename?) (file:///E:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calStorageHelpers.js)

Error: Error opening input stream (invalid filename?) (file:///E:/obj/sb-dbg/mozilla/dist/bin/calendar-js/upgrade.jsm)

In my opinion upgrade.jsm should be renamed to calStorageUpgrade.jsm to avoid potential namespace issues.

I'll try to fix the issue and proceed testing.

Comment 15

[Stefan Sitter [:ssitter]]

Attachment: Fix - v3

Updated patch that fixes the errors mentioned in comment #14, fixes "ASSERT is not defined"  in calStorageUpgrade.jsm, fixes "Assert failed: Database has not been opened!" in calStorageUpgrade.jsm.

But creating new profile still fails with:

[Exception... "'[JavaScript Error: "version is not defined" {file: "file:///E:/obj/sb-dbg/mozilla/dist/bin/modules/calStorageUpgrade.jsm" line: 804}]' when calling method: [calICalendar::uri]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///E:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calCalendarManager.js :: cmgr_createCalendar :: line 613"  data: yes]

Comment 16

[Stefan Sitter [:ssitter]]

If I add the |version| argument to all |upgrade_v*| function definitions and calls and now the creation proceeds further until it fails with "db.beginTransaction is not a function" in upgrade_v3().

Can you explain what the construct

  |  upgrade.v3 = function upgrade_v3(db, version) {        |
  |      let tbl = upgrade.v2(version < 2 && db, version);  |
  |      db.beginTransaction();                             |

in this and the other functions is supposed to do? Don't we always call the underling upgrade function with |false| as the first argument and therefore all references to |db| in that function fail?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 393875 [details] [diff] [review]
Fix - v2

Wow, I wonder how this patch could have ever worked for me! When I last tested this, I had no errors, but now I also have the same errors.

I have a local patch that takes care of all the issues, I'll need to test it again though.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #16)
> If I add the |version| argument to all |upgrade_v*| function definitions and
> calls and now the creation proceeds further until it fails with
> "db.beginTransaction is not a function" in upgrade_v3().
> 
> Can you explain what the construct
> 
>   |  upgrade.v3 = function upgrade_v3(db, version) {        |
>   |      let tbl = upgrade.v2(version < 2 && db, version);  |
>   |      db.beginTransaction();                             |
> 
> in this and the other functions is supposed to do? Don't we always call the
> underling upgrade function with |false| as the first argument and therefore all
> references to |db| in that function fail?

So the idea is the following: When upgrading, we need to get the table data for the current version, then apply changes both to the table data object and the actual SQL. Since I don't want to add an object for each schema version, I rather went for incremental changes. The flow is as follows. Lets say we are at db version 13 and upgrade to v16:


* upgrade.v16 calls v15 with a database object to get the current table data
  object and make sure the database is updated to v15.
* upgrade.v15 does the same until v13.
* upgrade.v13 is called:
** upgrade.v13 calls upgrade.v12 *without* a database, so that v12 can create 
  the tableData object for v12
** upgrade.v12 calls upgrade.v11 also to only get the table data object
** ... the same down to v1
** upgrade v13 both updates the table data object and does the actual db
  transactions to upgrade to v13

I hope this explains it, I need to go. I'll read again later in case I was unclear :-)

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 394059 [details] [diff] [review]
Fix - v3

Missing steps:

** upgrade.v14 updates the table data object and executes the database transactions to update to v14.
** The same for update.v15 and update.v16
** Result: The database is up to date.

I introduced a bunch of functions that do certain common things (i.e add a Column, change a type, remove a column), these update the database (if the current upgrader has one) and also the table data object. The current upgrader always has the current table data object this way.

I will add some more documentation to calStorageUpgrades.jsm and soon upload a new version.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v4

Wow, there was really a lot broken with my last patch. This patch should be a lot better. I tested storage.sdb's from 0.3, 0.3.1, 0.5, 0.7 and 0.9, they all work. They didn't contain many events though, just a simple event with all details and a simple allday event with all details. Some testing with recurring events and events with alarms would probably be nice.

Comment 21

[Martin Schröder [:mschroeder]]

Comment on attachment 395418 [details] [diff] [review]
Fix - v4

Quick code review (functional review hopefully follows soon):

>diff --git a/calendar/base/src/calCalendarManager.js b/calendar/base/src/calCalendarManager.js
>--- a/calendar/base/src/calCalendarManager.js
>+++ b/calendar/base/src/calCalendarManager.js
>@@ -627,7 +627,8 @@
>                     message = calGetString("calendar", "unknownTimezonesError", [uri.spec]);
>                     break;
>                 default:
>-                    message = calGetString("calendar", "unableToCreateProvider", [uri.spec]);
>+                  //  message = calGetString("calendar", "unableToCreateProvider", [uri.spec]) +
>+                   //     ex;
>                     break;
>             }
>             ASSERT(false, message);

Commented out code should be removed, not commented out or replaced by the original! ;)


>diff --git a/calendar/providers/storage/calStorageCalendarModule.js b/calendar/providers/storage/calStorageCalendarModule.js
>--- a/calendar/providers/storage/calStorageCalendarModule.js
>+++ b/calendar/providers/storage/calStorageCalendarModule.js
>@@ -71,10 +71,11 @@
>             return;
> 
>         Components.utils.import("resource://calendar/modules/calUtils.jsm");
>+
>         cal.loadScripts(["calUtils.js", "calStorageCalendar.js"],
>                         this.__parent__);
> 
>-        initCalStorageCalendarComponent();
>+
>         this.mUtilsLoaded = true;
>     },
> 

Remove one of the two empty lines in succession!


>diff --git a/calendar/providers/storage/calStorageUpgrade.jsm b/calendar/providers/storage/calStorageUpgrade.jsm
>new file mode 100644
>--- /dev/null
>+++ b/calendar/providers/storage/calStorageUpgrade.jsm
[...]
>+/**
>+ * Gets the SQL for the given table data and table name
>+ *
>+ * @param tblName       The name of the table to retrieve sql for
>+ * @param tblData       The table data object, as returned from the upgrade_v*
>+ *                        functions. If null, then the latest table data is
>+ *                        retrieved
>+ */
>+function getSql(tblName, tblData, alternateName) {
>+    tblData = tblData || getSqlTable();
>+    let sql = "CREATE TABLE " +  (alternateName || tblName) + " (\n";
>+    for (let [key, type] in Iterator(tblData[tblName]))  {
>+        sql += "    " + key + " " + type + ",\n";
>+    }
>+
>+    return sql.replace(/,\s*$/, ");");
>+}

Comment should also describe the argument 'alternateName'.


>+function getVersion(db) {
[...]
>+    } catch (e) {
>+        throw reportError(db, e);
>+    } finally {
>+        if (selectSchemaVersion) {
>+            selectSchemaVersion.reset();
>+        }
>+    }
>+    cal.WARN(cal.STACK(10));
>+
>+
>+    throw "cal_calendar_schema_version SELECT returned no results";
>+}

The 'throw' statement seems to be a leftover from testing and debugging.


>+function ensureUpdatedTimezones(db) {
[...]
>+        } catch (exc) {
>+            cal.ASSERT(false, "Timezone update failed! DB Error: " + lastErrorString(db));
>+            rollbackTransaction(db);
>+            throw exc;
>+        }
>+    }
>+}

Stay consistent using 'e' as variable for the caught exception. ;)

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #21)
> >+    cal.WARN(cal.STACK(10));
> >+
> >+
> >+    throw "cal_calendar_schema_version SELECT returned no results";
> >+}
> 
> The 'throw' statement seems to be a leftover from testing and debugging.
The throw statement is wanted, this comes from the previous code. The WARN statement is not needed though.

All other nits fixed locally.

Comment 23

[Stefan Sitter [:ssitter]]

I finally found some time for testing: Creating new profile now works. But I can't delete a single occurrence of a repeating event in Sunbird 1.0pre:

[[[
Error: DB error: not an error
exc: ReferenceError: UTC is not defined

An error occurred executing the calendar_delete_event_command command

[Exception... "'[JavaScript Error: "UTC is not defined" {file: "file:///D:/dev/obj/sb/mozilla/dist/bin/modules/calStorageHelpers.jsm" line:108}]' when calling method: [calICalendar::modifyItem]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JSframe :: file:///D:/dev/obj/sb/mozilla/dist/bin/calendar-js/calTransactionManager.js :: cT_doTransaction :: line 212"  data: yes]
]]]

I can't test the upgrade due to Bug 514234. The crash happens for me with nightly builds too. Therefore it is not related to your patch. If it matters: After the crash Sunbird starts but reports:

[[[
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.createTable]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///D:/dev/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js :: calmgr_checkAndMigrateDB :: line 494"  data: no]
Source File: file:///D:/dev/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js
Line: 494
]]]

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #23)
> I finally found some time for testing: Creating new profile now works. But I
> can't delete a single occurrence of a repeating event in Sunbird 1.0pre:
> 
> [[[
> Error: DB error: not an error
> exc: ReferenceError: UTC is not defined
Found this one. There is a call in calStorageHelpers that uses UTC() directly, not cal.UTC(). We don't load calUtils.js in the storage helpers, only calUtils.jsm.

> I can't test the upgrade due to Bug 514234. The crash happens for me with
> nightly builds too. Therefore it is not related to your patch. If it matters:
> After the crash Sunbird starts but reports:
> 
> [[[
> Error: [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [mozIStorageConnection.createTable]"  nsresult: "0x80004005
> (NS_ERROR_FAILURE)"  location: "JS frame ::
> file:///D:/dev/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js ::
> calmgr_checkAndMigrateDB :: line 494"  data: no]
> Source File:
> file:///D:/dev/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js
> Line: 494
> ]]]

I had this problem too, I've even seen it on a pure 0.9 profile recently. The problem there was that some upgrade partially worked, i.e cal_relations already existed, but the storage wasn't really upgraded. Therefore createTable fails since the table already exists.

In my case I had it when manually switching storage.sdb's into the current profile and thought it was related to some borked calendar manager upgrades. I'll see if I can reproduce this again. Can you attach your sqlite database with the broken createTable?

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Does your crash also happen with Lightning?

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

My personal createTable error is really just a borked profile, I get the error in calmgr_checkAndMigrateDB.

Comment 27

[Stefan Sitter [:ssitter]]

Today neither my own builds nor the nightly builds crash during the update but I have no idea what changed. However it doesn't seem to work correct. After update Sunbird is not really usable.

Update from 0.9 (just fresh profile) to 1.0pre: Error after startup, toolbar buttons are disabled. From Console:

[[[
Storage: Migrating storage.sdb -> local.sqlite
Storage: Preparing to upgrade v14 to v16
Storage: Upgrading to v15
Storage: Upgrading to v16
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.createTable]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///D:/dev/obj/sb-dbg/mozilla/dist/bin/calendar-js/calCalendarManager.js :: calmgr_checkAndMigrateDB :: line 494"  data: no]
************************************************************
]]]

Update from 0.3.1 to 1.0pre: Timezone information seems to be lost, switched from floating to UTC. From Console:

[[[
**** Upgrading calCalendarManager schema to 6
**** Upgrading calCalendarManager schema to 9/10
Storage: Migrating storage.sdb -> local.sqlite
Storage: Preparing to upgrade v5 to v16
Storage: Upgrading to v6
Storage: Upgrading to v7
Storage: Upgrading to v8
Storage: Upgrading to v9
Storage: Upgrading to v10
Storage: Upgrading to v11
Storage: Upgrading to v12
Storage: Upgrading to v13
Storage: Upgrading to v14
Storage: Upgrading to v15
Storage: Upgrading to v16
Timezones have been updated, updating calendar data.
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "window is not defined" {file: "file:///D:/dev/obj/sb-dbg/mozilla/dist/bin/calendar-js/calUtils.js" line: 985}]' when calling method: [calITimezoneService::version]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///D:/dev/obj/sb-dbg/mozilla/dist/bin/modules/calStorageUpgrade.jsm :: ensureUpdatedTimezones :: line 388"  data: yes]
************************************************************
]]]

After the next startup the same [mozIStorageConnection.createTable] error is logged as above for the 0.9 update.

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

This happens with a fresh Sunbird? I tested this with Lightning, upgrading both a fresh profile and a profile with 2 events from Lightning 0.9 to 1.0pre worked flawlessly. At one time the application hung though, killing and restarting also caused the createTable error.

I could surely add code to DROP the table before re-creating it there, but I'd still like to know why this is happening. I'll try again with Sunbird now.

Comment 29

[Stefan Sitter [:ssitter]]

Yes, I'm testing with Sunbird builds. Would it be useful if I attach the Sunbird profile files before and after update?

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v5

What an aggrivating bug! All I changed between v4 and v5 is the extra comments in calStorageUpgrades.jsm's upgrade.v1.

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

I couldn't quite reproduce the timezone stuff. I created an event in 0.3, exported it, removed the timezone information (i.e made it floating), then re-imported that event, then upgraded to 1.0pre with patches applied. Then I exported the event again, and it was still floating. Any different STRs ?

Comment 32

[Stefan Sitter [:ssitter]]

Further testing: Creating new Sunbird profile and upgrading from Sunbird 0.9 worked better with the v5 patch.

It seems the timezones already got converted to UTC when importing the test calendar <http://www.mozilla.org/projects/calendar/caldata/GermanHolidays.ics> into Sunbird 0.3.1. During the upgrade I got a popup window with the "window is not defined" error from comment #27. Maybe this error can be ignored?

During upgrade from Sunbird 0.3a1 (storage schema v4) I got a different error and alarms were lost. Otherwise it seems to work fine. Do we care about upgrade from releases earlier to 0.3.1? The error was: 

[[[
Storage: Preparing to upgrade v4 to v16
...
Storage: Upgrading to v16

Error: Error calling executeSimpleSQL db error: User function returned error code
Exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file://[...]/modules/calStorageUpgrade.jsm :: anonymous :: line 245"  data: no]

Warning: 
1: [file://[...]/modules/calStorageUpgrade.jsm:249] anonymous
2: [file://[...]/modules/calStorageUpgrade.jsm:1015] copyDataOver
3: [file://[...]/modules/calStorageUpgrade.jsm:1019] upgrade_v16
4: [file://[...]/modules/calStorageUpgrade.jsm:208] upgradeDB
5: [file://[...]/calendar-js/calStorageCalendar.js:715] cSC_initDB
6: [file://[...]/calendar-js/calStorageCalendar.js:199] anonymous
7: [null:0] null
8: [file://[...]/calendar-js/calCalendarManager.js:613] cmgr_createCalendar
9: [file://[...]/calendar-js/calCalendarManager.js:775] cmgr_assureCache
10: [file://[...]/calendar-js/calCalendarManager.js:742] cmgr_getCalendars
]]]

Comment 33

[Stefan Sitter [:ssitter]]

Comment on attachment 399444 [details] [diff] [review]
Fix - v5

>+++ b/calendar/base/src/calCalendarManager.js
>@@ -627,7 +627,8 @@
>   default:
>-      message = calGetString("calendar", "unableToCreateProvider", [uri.spec]);
>+      message = calGetString("calendar", "unableToCreateProvider", [uri.spec]) +
>+                ":" + message;

I don't think this change matches into the current text "An error was encountered preparing the calendar located at %1$S for use. It will not be available.". In my opinion the exception should be reported to error console and the popup text should stay as is. In addition the exception already seems to be contained in the Details section of the error popup.

>+++ b/calendar/providers/storage/Makefile.in
>+EXTRA_JS_MODULES = calStorageHelpers.jsm calStorageUpgrade.jsm

Following what I saw in other makefiles this probably should be written as:
    EXTRA_JS_MODULES = \
        calStorageHelpers.jsm \
        calStorageUpgrade.jsm \
        $(NULL)

>+++ b/calendar/providers/storage/calStorageHelpers.jsm
>+// bitmasks
>+var CAL_ITEM_FLAG = {

CAL_ITEM_FLAG seems to be used only in calStorageCalendar.js. Why do you declare it here instead of calStorageCalendar.js? And maybe the comment could be a little bit more descriptive :)

>+/**
>+ * Create a storage statement on the given connection with the passed
>+ * sql.
>+ *
>+ * @param aDb       The database to create the statement on.
>+ * @param aSql      The SQL of the statement to create.
>+ */
>+function createStatement(aDb, aSql) {

The comments seem incomplete. For example what is the "database"? Is it a string? A mozIStorageConnection object? Something different?

>+/**
>+ * Returns the passed date in UTC, unless it is floating. In this case, it is
>+ * kept floating.
>+ *
>+ * @param dt        The date to convert
>+ * @return          The possibly converted date
>+ */
>+function getInUtcOrKeepFloating(dt) {

The comments seem incomplete. What is dt? A Date object? A calIDateTime object?

>+/**
>+ * Creates a new calIDateTime from the given native time and optionally
>+ * the passed timezone.
>+ *
>+ * @param aNativeTime       The native time, in microseconds
>+ * @param aTimezone         The timezone identifier or definition
>+ */
>+function newDateTime(aNativeTime, aTimezone) {

The comments seem incomplete. Is aTimezone a VTIMEZONE string? A calITimezone object? Something different?

>+++ b/calendar/providers/storage/calStorageUpgrade.jsm
+ *     beginTransaction(db);
+ *     try {
+ *         // Do stuff here
+ *         setDbVersion(db, NN);
+ *     } catch (e) {
+ *         throw reportError(db, e);
+ *     }
+ *     return tbl;

For better readability and understanding of the upgrader code I suggest to rename the following functions:
    setDbVersion --> setDbVersionAndCommitTransaction
    reportError  --> reportErrorAndRollbackTransaction
Or maybe create two methods:
    reportErrorAndRollbackTransaction() that rollbacks and calls reportError()
This way the reader of the code knows what happens if reportError() is called in once place and reportErrorAndRollbackTransaction() in another place.

>+/**
>+ * Gets all SQL for the given table data
>+ *
>+ * @param tblData       The table data object, as returned from the upgrade_v*
>+ *                        functions. If null, then the latest table data is
>+ *                        retrieved
>+ */
>+function getAllSql(version) {
>+    let tblData = getSqlTable(version);

The comment doesn't match the function signature.

>+function getVersion(db) {
>+...
>+    try {
>+        selectSchemaVersion = createStatement(db,
>+                              "SELECT version FROM " +
>+                              "cal_calendar_schema_version LIMIT 1");
>+        if (selectSchemaVersion.step()) {
>+            version = selectSchemaVersion.row.version;
>+        }
>+        if (version !== null) {
>+            // This is the only place to leave this function gracefully.
>+            return version;
>+        }
>+    } catch (e) {

Is selectSchemaVersion.reset() required before the return? Or is the finally clause called before the return is executed?

>+function createDBDelegate(funcName) {

Is there more required to name the function in addition to setting func.name? The created functions are still logged as anonymous, see error message in comment #32.

>+    cal.ERROR("Error calling " + funcName + " db error: " +
>+              lastErrorString(db) + "\nException: " + e);
>+    cal.WARN(cal.STACK(10));

I was confused by the message created here. It is not clear what parts belong together. I'd change the message to make it more clear. From comment #32:

  Error: Error calling executeSimpleSQL db error: User function returned error code

  Error: Call to "executeSimpleSQL" failed. DB Error: "User function returned error code".

>+// These functions us the db delegate to allow easier calling of common database
>+// functions.
>+var beginTransaction = createDBDelegate("beginTransaction");
>+...

Nit: There seems to be a typo in the comment. Personally I'd group the functions that belong together, e.g. beginTransaction, commitTransaction, rollbackTransaction, ...

>+upgrade.v16 = function upgrade_v16(db, version) {
>+...
>+  // Make sure the alarm flag is set on the item
>+  executeSimpleSQL(db, "UPDATE cal_events SET flags = flags | 256 WHERE id IN" +
>+                   " (SELECT item_id " +
>+                   "    FROM cal_alarms " +
>+                   "   WHERE cal_alarms.cal_id = cal_events.cal_id)");
>+  executeSimpleSQL(db, "UPDATE cal_todos SET flags = flags | 256 WHERE id IN" +

What does the magic number 256 means? Maybe create a local var with a descriptive name and use it instead?

Comment 34

[Simon Paquet [:sipaq]]

(In reply to comment #32)
> During upgrade from Sunbird 0.3a1 (storage schema v4) I got a different 
> >error and alarms were lost. Otherwise it seems to work fine. Do we care 
> about upgrade from releases earlier to 0.3.1?

No, we don't. Someone who hasn't updated our software for more than 2.5 years 
will likely not update anyway.

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #33)
> I don't think this change matches into the current text "An error was
> encountered preparing the calendar located at %1$S for use. It will not be
> available.". In my opinion the exception should be reported to error console
> and the popup text should stay as is. In addition the exception already seems
> to be contained in the Details section of the error popup.
Changed this to be two separate messages, one being the orignal error, the other being the exception logged directly to the error console.


> CAL_ITEM_FLAG seems to be used only in calStorageCalendar.js. Why do you
> declare it here instead of calStorageCalendar.js? And maybe the comment could
> be a little bit more descriptive :)
I guess theoretically other users could import the storage helpers module and there use the item flags. Also, its now used in the upgraders too. I've made it a bit more descriptive.



> For better readability and understanding of the upgrader code I suggest to
> rename the following functions:
>     setDbVersion --> setDbVersionAndCommitTransaction
>     reportError  --> reportErrorAndRollbackTransaction
To not be too long, I stripped the Transaction part but otherwise used the suggested function names.


> Is selectSchemaVersion.reset() required before the return? Or is the finally
> clause called before the return is executed?
Finally is called before the return.

> I was confused by the message created here. It is not clear what parts belong
> together. I'd change the message to make it more clear. From comment #32:
Done

> Nit: There seems to be a typo in the comment. Personally I'd group the
> functions that belong together, e.g. beginTransaction, commitTransaction,
> rollbackTransaction, ...
Done

> What does the magic number 256 means? Maybe create a local var with a
> descriptive name and use it instead?
this is CAL_ITEM_FLAG.HAS_ALARMS. I've changed the code to use this directly.



(In reply to comment #32)
> It seems the timezones already got converted to UTC when importing the test
> calendar <http://www.mozilla.org/projects/calendar/caldata/GermanHolidays.ics>
> into Sunbird 0.3.1. During the upgrade I got a popup window with the "window is
> not defined" error from comment #27. Maybe this error can be ignored?
This happens because showError() wants a window to display a dialog alert. I've never liked this, maybe we can get rid of it, or at least only dump() when no window is defined. I think var window = window || null will get rid of the undefined error, I'll do this in the next iteration.


> 
> During upgrade from Sunbird 0.3a1 (storage schema v4) I got a different error
> and alarms were lost. Otherwise it seems to work fine. Do we care about upgrade
> from releases earlier to 0.3.1? The error was: 
In general I agree with Simon, if we get this for free though then I'll take care. Now that I know that 0.3a1 has v4, I'll do a very quick test and see if its trivial to fix.


Any other non-code problems to be aware of?

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v6

Patch for previous comment

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v7

Fixes 0.3a1 stuff, I've filed bug 520463 to fix a followup issue.

Comment 38

[Stefan Sitter [:ssitter]]

Comment on attachment 404508 [details] [diff] [review]
Fix - v7

For testing I imported the calendar localted at <http://www.sunbird-kalender.de/extension/kalender/Feiertage_2009-2013_DE.ics> into a clean Sunbird profile and upgraded to a patched 1.0pre build. During upgrade I got some exceptions and an error popup. But afterwards Sunbird starts normally and it seems the update was successfully.

Update from Sunbird 0.3a1:
==========================
OK

Update from Sunbird 0.3.1 / 0.5 / 0.7:
======================================
[Exception... "'[JavaScript Error: "this.mSelectByTzid is undefined" {file: "[...]/calendar-js/calTimezoneService.js" line: 233}]' when calling method: [calITimezoneService::getTimezone]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: [...]/modules/calStorageHelpers.jsm :: getTimezone :: line 214"  data: yes]

Update from Sunbird 0.8:
========================
[Exception... "'[JavaScript Error: "this.mSelectByTzid is undefined" {file: "[...]/calendar-js/calTimezoneService.js" line: 233}]' when calling method: [calITimezoneService::getTimezone]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: [...]/modules/calStorageUpgrade.jsm :: ensureUpdatedTimezones :: line 368"  data: yes]

Update from Sunbird 0.9:
========================
OK

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v8

Ugh, what an ugly fix. For some reason when upgrading the timezones extension is copied partially into the profile, on the first startup the timezones.sqlite is empty which causes the error.

The code manually looks through items list and finds the right install location. This may be wrong though, I'm not really happy.

Daniel, do you have an idea whats going wrong here?

Comment 40

[Daniel Boelzle [:dbo]]

I can only imagine this is a concurrency race between addon management and calendar startup. Does postponing calendar startup help in any way (if this is possible at all)?

Comment 41

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v9

Wow I can't believe this caused me weeks and weeks! Interestingly enough, we have the same Problems even without this patch. After turning on extension logging I found out I have an empty directorty at:

~/.mozilla/extensions/{718...uuid of sunbird...}/[e-mail]

It seems this empty directory is preferred over the app-global directory and since it contains no files, the extension looks like its not installed. On windows, you can find this directory in your $HOME\Application Data\Mozilla\extensions folder.

If you remove this directory, the upgrade runs flawlessly. Now I wonder if this empty directory is something that happened because we have development profiles, or if this is some glitch that happened somewhere in the past and affects all users.

This patch is bascially v7 with a few style nits fixed and is ready for review.

Comment 42

[Stefan Sitter [:ssitter]]

Comment on attachment 409604 [details] [diff] [review]
Fix - v9

r=ssitter
Let's go with this patch and see if someone complains.

Comment 43

[Philipp Kewisch [:Fallen][📱📆]]

What an epic story! Thank you all for bearing with me on this one, for all the great testing and error hunting. This calls for a round of beers next time we see each other!

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9a6c1c951f94>
and comm-1.9.1 < <http://hg.mozilla.org/releases/comm-1.9.1/rev/1f724cccd665>

-> FIXED

Comment 44

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".