Closed Bug 630730 Opened 9 years ago Closed 9 years ago

Let consumers who probably know what they are doing access the database connection

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- -

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1.0 (obsolete) — Splinter Review
This is a bit of a hack, but we should let consumers who feel like they know what they are doing access the database connection of the password manager.  Scary, I know, but in the case of Sync, it really really wants to have a transaction around a bunch of calls it makes to add logins to the database.  This let's it start and end transactions and the password manager doesn't need any new public API.

This should probably be a softblocker, as it's a big problem for mobile with Sync.
Attachment #508956 - Flags: review?(dolske)
blocking2.0: --- → ?
Whiteboard: [needs review dolske][softblocker?]
This is not a blocker but presumably there is a blocker somewhere that requires you to do this evilness right?
blocking2.0: ? → -
Whiteboard: [needs review dolske][softblocker?] → [needs review dolske]
(In reply to comment #1)
> This is not a blocker but presumably there is a blocker somewhere that requires
> you to do this evilness right?

Correct. Filed bug 631001, nominated for blocking Fennec.
(In reply to comment #1)
> This is not a blocker but presumably there is a blocker somewhere that requires
> you to do this evilness right?
Yes, sorry, I didn't put everything in the bug.  This seems to help first Sync times on mobile (well, bug 631001 does, which needs this) quite a bit, which is a priority for the mobile team.
Comment on attachment 508956 [details] [diff] [review]
v1.0

One small nit:

Since we're exposing _dbConnection now, _dbCleanup() in storage-mozStorage.js should set _dbConnection to null after closing the connection. [This is only getting called when storage fails to init, which causes an exception, which means pwmgr's this._storage will throw on access, which means you can't _currently_ get a bad DB handle. But nulling it is a good safetybelt.]

Also, note that addons can make password manager use an alternate backend. Sync probably ought to gracefully degrade to the slow path, or at the very least not explode if the getInterface() call fails. :)
Attachment #508956 - Flags: review?(dolske)
Attachment #508956 - Flags: review+
Attachment #508956 - Flags: approval2.0+
Whiteboard: [needs review dolske]
(In reply to comment #4)
> Also, note that addons can make password manager use an alternate backend. Sync
> probably ought to gracefully degrade to the slow path, or at the very least not
> explode if the getInterface() call fails. :)
It already has to support 3.6, so it should be OK.  philikon is cc'd here, so I'm sure he's aware now.
Attached patch v1.1Splinter Review
Addresses review comments.
Attachment #508956 - Attachment is obsolete: true
Attachment #509517 - Flags: approval2.0?
Attachment #509517 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/ab87d34e89fa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.