Closed Bug 735912 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: liuche, Assigned: wesj)

References

Details

(Whiteboard: [sync])

Attachments

(7 files)

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.
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.
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.
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.
Blocks: 709385
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...
Nom'ing for triage.  this will block password sync work: bug 709385
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
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.
Whiteboard: [sync]
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.
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.
Assignee: nobody → wjohnston
Attachment #606366 - Flags: review?(mark.finkle)
This adds a delayedActions object that builds its own queue of things to... delay.
Attachment #606367 - Flags: review?(mark.finkle)
Comment on attachment 606366 [details] [diff] [review]
Create the databases during startup

>+  isUpdated: function() {

isAppUpdated

>+  onUpdate: function() {

onAppUpdated
Attachment #606366 - Flags: review?(mark.finkle) → review+
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
Attachment #606367 - Flags: review?(mark.finkle) → review-
https://hg.mozilla.org/mozilla-central/rev/2a46d0a93816
https://hg.mozilla.org/mozilla-central/rev/3849a7572a98
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Attached patch PatchSplinter Review
Landing on a Monday morning is apparently not advisable for me.
Attachment #608482 - Flags: review?(bnicholson)
Attachment #608482 - Flags: review?(bnicholson) → review+
backout for test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/197cccd053e9

will post a test fix in a second
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Test FixesSplinter Review
This should fix the tests. Works locally. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7370272686db
Attachment #610756 - Flags: review?(mark.finkle)
Attachment #610756 - Flags: review?(mark.finkle) → review+
(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.
is this an issue?
Back on try:

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

I'l hopefully reland today sometime.
https://hg.mozilla.org/mozilla-central/rev/56d86fb6b887
https://hg.mozilla.org/mozilla-central/rev/af9989ded605
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.