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)
Toolkit
Password Manager
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)
4.88 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•13 years ago
|
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #509643 -
Flags: review?(dolske)
Assignee | ||
Comment 2•13 years ago
|
||
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=9b053ab2d5dd
Reporter | ||
Comment 3•13 years ago
|
||
FWIW I pushed this bug and bug 631001 to try yesterday: http://tbpl.mozilla.org/?tree=MozillaTry&rev=2ddda5ac4817
Assignee | ||
Comment 4•13 years ago
|
||
(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?
Updated•13 years ago
|
tracking-fennec: ? → 2.0b5+
Reporter | ||
Updated•13 years ago
|
Whiteboard: [has patch][needs review dolske]
Comment 5•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [has patch][needs review dolske] → [has patch]
Updated•13 years ago
|
Whiteboard: [has patch] → [has review][needs new patch]
Assignee | ||
Comment 6•13 years ago
|
||
Addresses review comments, still passes tests.
Attachment #509643 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has review][needs new patch] → [has review][can land]
Reporter | ||
Comment 7•13 years ago
|
||
Landed in places: http://hg.mozilla.org/projects/places/rev/7df11fdfc215
Whiteboard: [has review][can land] → [fixed in places]
Comment 8•13 years ago
|
||
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.
Description
•