564268 - Make calendar code calling the local database more robust

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Simon Vaillancourt]

The goal of this bug is to make the calendar code calling the database more robust by:
- Making sure all re-usable statements are reset() in a finally block
- Making sure all non-reusable statements have a finalize() in a finally block
- Validate that all the beginTransaction are either commited and/or rolled back.

These common bugs can lead to "database table is locked" and "library routine called out of sequence" errors.

Comment 1

[Simon Vaillancourt]

Attachment: Patch v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 445346 [details] [diff] [review]
Patch v1

>+                stmt.execute();
>+            }
>+            finally {

} finally {
here and elsewhere



>-                let attachStatement = createStatement(this.mDB, "ATTACH DATABASE :file_path AS local_sqlite");
>+                let attachStatement = this.mDB.createStatement("ATTACH DATABASE :file_path AS local_sqlite");
...
>-                            stmt = createStatement(db, "UPDATE " + tbl +
>+                            stmt = db.createStatement( "UPDATE " + tbl +
>                                                        "   SET cal_id = :cal_id" +
>                                                        " WHERE cal_id = :old_cal_id");
Why use the database's createStatement instead of our helper?



>diff --git a/calendar/providers/storage/calStorageUpgrade.jsm b/calendar/providers/storage/calStorageUpgrade.jsm
>-            selectSchemaVersion.reset();
>+            selectSchemaVersion.finalize();
...
>-        selectTzVersion.reset();
>+        selectTzVersion.finalize();
...
>-            getZones.reset();
>+            getZones.finalize();


Does finalize work here? I believe finalize doesn't work on the wrapper, just on its inner statement?



r=philipp with the above explained.

Comment 3

[Simon Vaillancourt]

Reverted to using the wrappers in the createStatement and "reset" in the upgrade script, after re-reading the finalize() doc on MDC I realized it was not mandatory since it should be called once the ref count reaches 0. 

Note that there is now a note on https://developer.mozilla.org/en/mozIStorageStatementWrapper that says that the wrapper interface is pretty much deprecated, maybe we should file a bug to track that for Lighting.

pushed to comm-central : http://hg.mozilla.org/comm-central/rev/9a29b14426d9

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/de4f7fd52dc4>

a=philipp