Closed Bug 564268 Opened 11 years ago Closed 11 years ago

Make calendar code calling the local database more robust

Categories

(Calendar :: Provider: Local Storage, defect)

Lightning 1.0b1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nomisvai, Assigned: nomisvai)

Details

Attachments

(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 → simon.at.orcl
Attached patch Patch v1Splinter Review
Attachment #445346 - Flags: review?(philipp)
Status: NEW → ASSIGNED
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 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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
You need to log in before you can comment on or make changes to this bug.