Closed Bug 754740 Opened 9 years ago Closed 9 years ago

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

Categories

(Calendar :: Internal Components, defect)

Lightning 1.7
defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mmecca, Assigned: mmecca)

References

Details

Attachments

(1 file, 4 obsolete files)

mozIStorageStatementWrapper has apparently been removed in Bug 589032
Version: unspecified → Lightning 1.7
Assignee: nobody → matthew.mecca
Status: NEW → ASSIGNED
Attached patch Fix v1 (obsolete) — Splinter Review
Adapt to removal of mozIStorageStatementWrapper.
Attachment #624285 - Flags: review?(mohit.kanwal)
Depends on: 589032
Duplicate of this bug: 755733
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.
(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...
(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 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 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]
(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.
Attached patch Fix v2 (obsolete) — Splinter Review
Removes createStatement function wrappers.

timezones.sqlite update is untested, I wasn't able to download the Olson database from elsie.nci.nih.gov.
Attachment #624285 - Attachment is obsolete: true
Attachment #624285 - Flags: review?(mohit.kanwal)
Attachment #625347 - Flags: review?(mohit.kanwal)
> 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 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.
(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.
No longer depends on: 589032
Depends on: 589032
Attachment #625347 - Flags: review?(mohit.kanwal)
Attached patch Fix v3 (obsolete) — Splinter Review
Replaces step with executeStep.
Attachment #625347 - Attachment is obsolete: true
Attachment #625459 - Flags: review?(mohit.kanwal)
(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.
Timezones version update appears to work fine also.
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.
Attached patch Fix v4 (obsolete) — Splinter Review
Fixes scope and exception issue in comment #17.

I'll ask review from the first available reviewer.
Attachment #625459 - Attachment is obsolete: true
Attachment #625459 - Flags: review?(mohit.kanwal)
Attachment #625823 - Flags: review?(ssitter)
Attachment #625823 - Flags: review?(mohit.kanwal)
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.
Attachment #625823 - Flags: review?(mohit.kanwal) → review+
(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.
Attached patch Fix v5Splinter Review
Final version for check-in
Attachment #625823 - Attachment is obsolete: true
Attachment #625823 - Flags: review?(ssitter)
Attachment #625859 - Flags: review+
ok have tested the patch with an older timezones.sqlite and it works.
Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/e098241ed032
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.7
You need to log in before you can comment on or make changes to this bug.