Closed Bug 564268 Opened 12 years ago Closed 12 years ago

Make calendar code calling the local database more robust


(Calendar :: Provider: Local Storage, defect)

Lightning 1.0b1
Not set


(Not tracked)



(Reporter: nomisvai, Assigned: nomisvai)



(1 file)

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.
Assignee: nobody →
Attached patch Patch v1Splinter Review
Attachment #445346 - Flags: review?(philipp)
Component: General → Provider: Local Storage
QA Contact: general → storage-provider
Hardware: x86 → All
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.
Attachment #445346 - Flags: review?(philipp) → review+
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 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 :
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
You need to log in before you can comment on or make changes to this bug.