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

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

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.

Comment 1

[Dave Townsend [:mossop] (he/him)]

This is not a blocker but presumably there is a blocker somewhere that requires you to do this evilness right?

Comment 2

[Philipp von Weitershausen [:philikon]]

(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.

Comment 3

[Shawn Wilsher :sdwilsh]

(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 4

[Justin Dolske [:Dolske]]

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. :)

Comment 5

[Shawn Wilsher :sdwilsh]

(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.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments.

Comment 7

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/ab87d34e89fa