Closed Bug 631403 Opened 13 years ago Closed 13 years ago

LoginManagerStorage_mozStorage should be able to deal with an existing transaction

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b5+ ---

People

(Reporter: philikon, Assigned: sdwilsh)

References

Details

(Whiteboard: [fixed in places])

Attachments

(1 file, 1 obsolete file)

In bug 631001, we want to have Sync start and end the DB transaction to avoid fsyncs after every call into nsILoginManager. This should help mobile performance a lot (it already has for form data sync, see bug 630720).

However, LoginManagerStorage_mozStorage does not handle an existing transaction gracefully when it calls beginTransaction().
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #509643 - Flags: review?(dolske)
(In reply to comment #3)
> FWIW I pushed this bug and bug 631001 to try yesterday:
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=2ddda5ac4817
double the coverage, double the fun?
tracking-fennec: ? → 2.0b5+
Whiteboard: [has patch][needs review dolske]
Comment on attachment 509643 [details] [diff] [review]
v1.0

The only transaction here that should be involved with Sync is the reencryptBase64Logins() case -- the other are performed in DB upgrades on startup, and will be finished before a caller gets a connection handle. And so it's tempting to suggest a simpler patch of deferring the transaction start there (as this patch does). But Sync could still possibly catch that transaction (though it should be rare), and we've already fiddled with this function a few times to make it not sucky in cases we thought were rare but were actually not.

So, tl;dr, let's do what this patch does.


>+Transaction.prototype = {
>+    commit : function() {
>+        if (this._hasTransaction) {
>+            this._db.commitTransaction();
>+        }

No braces needed here for single-line if, per existing style.

>+    rollback : function() {
>+        if (this._hasTransaction) {
>+            this._dbConnection.rollbackTransaction();
>+        }
>+    },

I think you want |this._db| here, not this._dbConnection!

r+ with the above.
Attachment #509643 - Flags: review?(dolske) → review+
Whiteboard: [has patch][needs review dolske] → [has patch]
Whiteboard: [has patch] → [has review][needs new patch]
Attached patch v1.1Splinter Review
Addresses review comments, still passes tests.
Attachment #509643 - Attachment is obsolete: true
Whiteboard: [has review][needs new patch] → [has review][can land]
Landed in places: http://hg.mozilla.org/projects/places/rev/7df11fdfc215
Whiteboard: [has review][can land] → [fixed in places]
Landed in m-c:
http://hg.mozilla.org/mozilla-central/rev/7df11fdfc215
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: