754740 - Startup failure since Bug 589032 - Error: Error setting up timezone database: TypeError: Components.classes['@mozilla.org/storage/statement-wrapper;1'] is undefined

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Matthew Mecca [:mmecca]]

mozIStorageStatementWrapper has apparently been removed in Bug 589032

Comment 1

[Stefan Sitter [:ssitter]]

Some information that might help with the change: http://blog.mozilla.org/nfroyd/2012/05/14/statement-wrappers-have-been-deprecated/

Comment 2

[Matthew Mecca [:mmecca]]

Attachment: Fix v1

Adapt to removal of mozIStorageStatementWrapper.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 624285 [details] [diff] [review]
Fix v1

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

::: calendar/timezones/update.js
@@ +314,5 @@
>                  params.component = tz.icalComponent.serializeToICS().replace(/\r\n\r\n/g, "\r\n");
>              } else { // alias
>                  params.alias = tz.tzid;
>              }
> +            statement.executeStep();

You should double-check whether you actually want to s/execute/executeStep/g.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #4)
> You should double-check whether you actually want to s/execute/executeStep/g.

As in that seems to be a mistake in the blog...

Comment 6

[Matthew Mecca [:mmecca]]

(In reply to Serge Gautherie (:sgautherie) from comment #4)

> > +            statement.executeStep();
> 
> You should double-check whether you actually want to s/execute/executeStep/g.

From my understanding mozIStorageStatement.execute() does the same as .executeStep() followed by .reset(), and since we already do the reset in the finally blocks it makes sense to me that executeStep would be the proper replacement.

Comment 7

[Martin Schröder [:mschroeder]]

Comment on attachment 624285 [details] [diff] [review]
Fix v1

> function createStatement(dbconn, sql) {
>-    let stmt = dbconn.createStatement(sql);
>-    let wrapper = Components.classes["@mozilla.org/storage/statement-wrapper;1"]
>-                            .createInstance(Components.interfaces.mozIStorageStatementWrapper);
>-    wrapper.initialize(stmt);
>-    return wrapper;
>+    return dbconn.createStatement(sql);
> }

In my opinion, it does not make sense to wrap a function call in an one line function. I observed this multiple times in this patch, some times with an additional try-catch block which could also be moved directly in the calling function.

Comment 8

[Mohit Kanwal [:redDragon]]

Comment on attachment 624285 [details] [diff] [review]
Fix v1

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

(In reply to Matthew Mecca [:mmecca] from comment #6)
> From my understanding mozIStorageStatement.execute() does the same as
> .executeStep() followed by .reset(), and since we already do the reset in
> the finally blocks it makes sense to me that executeStep would be the proper
> replacement.

From the blog, execute() is replaced by executeStep() so that's fine. 

Also I am wondering does this mean there need to be changes done to the offline part of the storage code? e.g. in [1] there is statement.step() used. That should be fine right?

Waiting for a bit of　clarification on the execute and executeStep issue as posted on Nathan's blog by Serge Gautherie in the comments section.
 
[1] http://mxr.mozilla.org/comm-central/source/calendar/providers/storage/calStorageCalendar.js#897

::: calendar/providers/storage/calStorageCalendar.js
@@ +431,5 @@
> +     * @param aId           The id of the item.
> +     */
> +    executeItemStatement: function cSC_executeItemStatement(aStmt, aIdParam, aId) {
> +        try {
> +            aStmt.params.cal_id = this.id;

there is function to do this: this.prepareStatement(aStmt)

::: calendar/timezones/update.js
@@ +314,5 @@
>                  params.component = tz.icalComponent.serializeToICS().replace(/\r\n\r\n/g, "\r\n");
>              } else { // alias
>                  params.alias = tz.tzid;
>              }
> +            statement.executeStep();

This should be fine, Nathan's blog says that execute() be replaced by executeStep(), rest all calls should remain the same. [mohit]

Comment 9

[Matthew Mecca [:mmecca]]

(In reply to Martin Schröder [:mschroeder] from comment #7)
> In my opinion, it does not make sense to wrap a function call in an one line
> function. I observed this multiple times in this patch, some times with an
> additional try-catch block which could also be moved directly in the calling
> function.

Good point, I'll upload a new version of the patch.

> ::: calendar/providers/storage/calStorageCalendar.js
> @@ +431,5 @@
> > +     * @param aId           The id of the item.
> > +     */
> > +    executeItemStatement: function cSC_executeItemStatement(aStmt, aIdParam, aId) {
> > +        try {
> > +            aStmt.params.cal_id = this.id;
> 
> there is function to do this: this.prepareStatement(aStmt)

The prepareStatement function as it is now doesn't set the item id parameter, or execute and reset the statement. Also it sets the mLastStatement, which shouldn't be necessary for these calls. We could consolidate this into prepareStatement and act accordingly based on the function parameters passed in, but in my opinion using a separate function in this case makes the code easier to understand: prepareStatement when we plan to retrieve results, executeItemStatement when we are doing a one shot statement execution to modify an item row.

Comment 10

[Matthew Mecca [:mmecca]]

Attachment: Fix v2

Removes createStatement function wrappers.

timezones.sqlite update is untested, I wasn't able to download the Olson database from elsie.nci.nih.gov.

Comment 11

[Stefan Sitter [:ssitter]]

> timezones.sqlite update is untested, I wasn't able to download
> the Olson database from elsie.nci.nih.gov.

The current home for the tz database is https://www.iana.org/time-zones.

You could take timezones.sqlite from an older Lightning package for testing. Or create a profile in old Thunderbird + Lightning and than switch to new Thunderbird + Lightning to test storage update too.

Comment 12

[Stefan Sitter [:ssitter]]

Comment on attachment 625347 [details] [diff] [review]
Fix v2

>+++ b/calendar/base/src/calCalendarManager.js
>-            stmt = createStatement(db, "SELECT version FROM " + table + " LIMIT 1");
>+            stmt = db.createStatement("SELECT version FROM " + table + " LIMIT 1");
>             if (stmt.step()) {

As far as I can tell step() doesn't exists in mozIStorageStatement and should be replaced with executeStep() here and in all other locations.

Comment 13

[Matthew Mecca [:mmecca]]

(In reply to Stefan Sitter from comment #12)
> As far as I can tell step() doesn't exists in mozIStorageStatement and
> should be replaced with executeStep() here and in all other locations.

step() still seems to be working for now, and it's documented in the mdn docs 
https://developer.mozilla.org/en/mozIStorageStatement#step%28%29 
but that may be out of date.

In any case I agree we should replace it.

Comment 14

[Matthew Mecca [:mmecca]]

Attachment: Fix v3

Replaces step with executeStep.

Comment 15

[Matthew Mecca [:mmecca]]

(In reply to Stefan Sitter from comment #11)
> You could take timezones.sqlite from an older Lightning package for testing.
> Or create a profile in old Thunderbird + Lightning and than switch to new
> Thunderbird + Lightning to test storage update too.

Profile update from 1.4 seems to work fine.

Comment 16

[Matthew Mecca [:mmecca]]

Timezones version update appears to work fine also.

Comment 17

[Stefan Sitter [:ssitter]]

Comment on attachment 625459 [details] [diff] [review]
Fix v3

>-                let attachStatement = createStatement(this.mDB, "ATTACH DATABASE :file_path AS local_sqlite");
>+
>                 try {
>+                    let attachStatement = this.mDB.createStatement("ATTACH DATABASE :file_path AS local_sqlite");
>                     attachStatement.params.file_path = localDB.databaseFile.path;
>-                    attachStatement.execute();
>+                    attachStatement.executeStep();
>                 } catch (exc) {
>                     this.logError("prepareInitDB attachStatement.execute exception", exc);
>                     throw exc;
>                 } finally {
>-                    attachStatement.reset();
>+                    if (attachStatement) {
>+                        attachStatement.reset();
>+                    }

I'm not sure this will work because |attachStatement| is only declared inside the try block. Similar code above and below move |let attachStatement;| outside the try scope but keep |createStatement| inside the try scope.

>+    executeItemStatement: function cSC_executeItemStatement(aStmt, aIdParam, aId) {
>+        try {
>+            aStmt.params.cal_id = this.id;
>+            aStmt.params[aIdParam] = aId;
>+            aStmt.executeStep();
>+        } catch (e) {
>+            this.logError("executeItemStatement exception", e);
>+        } finally {
>+            aStmt.reset();
>+        }
>+    },

The exception should not be swallowed up here. It should be re-thrown to trigger the existing transaction / rollback code.

Comment 18

[Matthew Mecca [:mmecca]]

Attachment: Fix v4

Fixes scope and exception issue in comment #17.

I'll ask review from the first available reviewer.

Comment 19

[Mohit Kanwal [:redDragon]]

Comment on attachment 625823 [details] [diff] [review]
Fix v4

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

Sorry for doing a late review was tied up over the weekend. I have tested the patch, works fine on a just built daily.app I will test it with my older profile timezones.sql to see if it works as well. I have just 2 very simple questions after which I think the patch is good to go. Setting + for review in the meantime.

::: calendar/base/src/calTimezoneService.js
@@ +331,5 @@
>  
>      get timezoneIds() {
>          if (!this.mTzids) {
>              var tzids = [];
> +            var selectAllButAlias = this.mDb.createStatement("SELECT * FROM tz_data WHERE alias IS NULL");

shouldn't this be let? I thought the official take was to change vars to let but I may be wrong.

::: calendar/timezones/update.js
@@ +226,5 @@
>          }
>      } finally {
> +        if (statement) {
> +            statement.reset();
> +        }

this block does not have a catch statement or a throw. Do we need it? I am just curious to know.

Comment 20

[Matthew Mecca [:mmecca]]

(In reply to Mohit Kanwal [:redDragon] from comment #19)
> ::: calendar/base/src/calTimezoneService.js
> @@ +331,5 @@
> >  
> >      get timezoneIds() {
> >          if (!this.mTzids) {
> >              var tzids = [];
> > +            var selectAllButAlias = this.mDb.createStatement("SELECT * FROM tz_data WHERE alias IS NULL");
> 
> shouldn't this be let? I thought the official take was to change vars to let
> but I may be wrong.

You're right, I just missed it. I'll fix before pushing.
 
> ::: calendar/timezones/update.js
> @@ +226,5 @@
> >          }
> >      } finally {
> > +        if (statement) {
> > +            statement.reset();
> > +        }
> 
> this block does not have a catch statement or a throw. Do we need it? I am
> just curious to know.

That whole code block is contained in an outer try block, I believe any exception will be caught by the outer catch clause. Using the inner try block just ensures that the inner finally clause executes.

Comment 21

[Matthew Mecca [:mmecca]]

Attachment: Fix v5

Final version for check-in

Comment 22

[Mohit Kanwal [:redDragon]]

ok have tested the patch with an older timezones.sqlite and it works.

Comment 23

[Matthew Mecca [:mmecca]]

Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/e098241ed032