735912 - Passwords database is not accessible for Sync through Content Provider on clean install.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Log 1: Database fetch error on first sync, no passwords saved.

Found while implementing password syncing in Android Sync.

1. Apply patch changes.
2. Start Fennec (to create ContentProviders)
3. Pair device to desktop Firefox with Sync.
4. Syncing starts.

Expected result:
Checking passwords database availability succeeds (fetch for a guid).

Actual result:
Null Cursor Exception is thrown. This indicates the column wasn't found.

Note: If a password is saved before setting up sync, the passwords database check succeeds.

Is the passwords database created lazily? It needs to be accessible on start, because users may set up sync w/o saving any passwords.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Log 2: Success on passwords database fetch after saving a password in Fennec.

Comment 2

[Wesley Johnston (:wesj)]

Yes, the db is created and upgraded lazily. If Gecko is running, we fire off a message to it and create the database (i.e. you can retry soon after and things should succeed). Otherwise, there's not much we can do without starting Gecko (unless we want to duplicate all the database creation and migration code in Java).

If sync wants some method to determine the difference between these two positions, we can provide that.

Comment 3

[Wesley Johnston (:wesj)]

Form History should be expecting this same problem (I think?), so I'm cc'ing nalexander to make sure we're aware that it can happen there too.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Passwords Sync patch WIP

Comment 5

[Chenxia Liu [:liuche]]

wesj, I think that would be fine as a workaround. Please provide a method for sync to check whether a db exists and fire off db creation if not; we can skip the initial passwords sync and reschedule.

Comment 6

[Wesley Johnston (:wesj)]

Sorry. I don't think expressed that quite right.

If
1.) sync tries to do anything with the Content Provider,
2.) the database doesn't exist or is the wrong version, and
3.) Gecko is running

we will fire off the database creation for you. You should be able to try again very soon after (within seconds) and things will work. Maybe we should also just force database creation on first run as well. I think we can do that (just a bit worried about startup impact).

If Gecko is not running, we don't do anything. You can keep trying, and we will keep failing. I've wondered if you guys need some way to query if Gecko is running. Are you looking for a way to start Gecko from Sync? I don't think we want to do that...

Comment 7

[Tony Chung [:tchung]]

Nom'ing for triage.  this will block password sync work: bug 709385

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - it seems that the password DB is not created by Firefox until a password is saved via the UI. That's not good. Looks like we need an explicit "make the DB" method. If we get that method, we could try to do the silent install of the DB.

Comment 9

[Wesley Johnston (:wesj)]

OK. Sounds like we want two things:

1.) Set up these databases earlier, without the user having to do something. That means, every time Fennec is upgraded/installed, we should do this at (delayed) startup. We can do this in either Java or JS, but I really want to make sure we only hit it on those "special" startups.

2.) Add a new content provider so that sync can query what's going on. filed Bug 736216 for that.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Create the databases during startup

Using a trick similar to what BrowserCLH does:

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/BrowserCLH.js#87

Initializing the passwords database is pretty slow. 2-3s on my phone (and my phone's file system is usually pretty fast). I have a follow up to put these in a setTimeout and delay them.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Move to delayed startup

This adds a delayedActions object that builds its own queue of things to... delay.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 606366 [details] [diff] [review]
Create the databases during startup

>+  isUpdated: function() {

isAppUpdated

>+  onUpdate: function() {

onAppUpdated

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 606367 [details] [diff] [review]
Move to delayed startup


>+var DelayedActions = {

Let's hold off on this for now. We should consider using it for more in a separate bug. We should also try timing the affect on Ts.

>-    // Init LoginManager
>-    Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
>-    // Init FormHistory
>-    Cc["@mozilla.org/satchel/form-history;1"].getService(Ci.nsIFormHistory2);

Are we sure we can remove these?

if we can safely remove these inits, then let's do it. Also:

916     } else if (aTopic == "Passwords:Init") {
917       var storage = Components.classes["@mozilla.org/login-manager/storage/mozStorage;1"].  
918         getService(Components.interfaces.nsILoginManagerStorage);

923     } else if (aTopic == "FormHistory:Init") {
924       var fh = Components.classes["@mozilla.org/satchel/form-history;1"].  
925         getService(Components.interfaces.nsIFormHistory2);
926       var db = fh.DBConnection;

fix this code to use "Cc" and "Ci" and one-line and "var" -> "let"

r- for new patch

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a46d0a93816

Comment 15

[Wesley Johnston (:wesj)]

eek. backout the bad bits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3849a7572a98

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2a46d0a93816
https://hg.mozilla.org/mozilla-central/rev/3849a7572a98

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch

Landing on a Monday morning is apparently not advisable for me.

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba795535c17

thanks brian!

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8ba795535c17

Comment 20

[Wesley Johnston (:wesj)]

backout for test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/197cccd053e9

will post a test fix in a second

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Test Fixes

This should fix the tests. Works locally. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7370272686db

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #21)
> Created attachment 610756 [details] [diff] [review]
> Test Fixes
> 
> This should fix the tests. Works locally. Pushed to try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=7370272686db

Note: This try push did not include robocop. If in doubt, just run all the tests for Android Opt.

Comment 23

[Allison Naaktgeboren :ally]

is this an issue?

Comment 24

[Wesley Johnston (:wesj)]

Back on try:

https://tbpl.mozilla.org/?tree=Try&rev=2ed200d7a807

I'l hopefully reland today sometime.

Comment 25

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af9989ded605
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d86fb6b887

Try looks good! Relanded with tests fixes.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/56d86fb6b887
https://hg.mozilla.org/mozilla-central/rev/af9989ded605