Closed Bug 715550 Opened 8 years ago Closed 8 years ago

Migration of Sync settings from XUL Fennec

Categories

(Firefox for Android :: General, defect, P2)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- soft
fennec 11+ ---

People

(Reporter: rnewman, Assigned: gcp)

References

Details

(Whiteboard: [sync])

Attachments

(2 files, 2 obsolete files)

It should be possible for Fennec to create an Android Sync account during profile migration: all of the user's credentials live in prefs and password manager in XUL Fennec.

This will avoid some "why do I have to set up Sync again?" complaints, I imagine.
Assignee: nobody → gpascutto
tracking-fennec: --- → 11+
Priority: -- → P3
Priority: P3 → P2
Comments from rnewman:

investigative approach is about:config, search for services.sync
passwords are the two topmost passwords in pwmgr
the things you care about are services.sync.{serverURL,username,client.*}
most likely we need to expose an intent for you to create an account
Note that I don't suggest migrating the pref-stored engine timestamps or syncIDs; just the core defining credentials for the account. I'd rather incur the expense of a full merge sync than risk a logic difference causing a bug.

That said, I sure hope the profile migrator preserves GUIDs and modification times!

Core defining features are (at least, I might have forgotten some):

Credentials:
  * Account  (services.sync.account)
  * Password (in pwmgr: "chrome://weave (Mozilla Services Password)")
  * Sync key (in pwmgr: "chrome://weave (Mozilla Services Encryption Passphrase)")

Client fields:
  * Client name (services.sync.client.name)
  * Client ID   (services.sync.client.GUID)

Server config:
  * services.sync.serverURL
  * services.sync.clusterURL (might as well save a round-trip)

We don't currently respect them, but also engine enabled status:

  services.sync.engine.[a-zA-Z0-9]* = boolean

Marina's working on the clients engine until the passwords content provider is available, so we'll see if we can follow that by an intent, so that you've actually got a place to write the data you're fetching.

If you're in the mood to work on that yourself, let me know when you get to it!
Attached patch This approach won't work. (obsolete) — Splinter Review
Just saving this here in case someone needs to do something similar. We can't actually ask Gecko for sync settings because Sync isn't compiled in, so it won't be available in the Preferences service. We need to parse user.prefs directly instead.
Attached patch Patch 1. WIP (obsolete) — Splinter Review
Well, turns out I was being a bit stupid. While querying the services.sync settings in Gecko, with Sync compiled out, indeed doesn't return anything useful, something useful *will* be returned if there are actual user settings present (and which I brilliantly forgot). 

This patch works similar to GeckoPreferences. But it needs the password manager to be complete and some hookups to transfer the retrieved settings to Sync.
Attachment #595055 - Attachment is obsolete: true
Comments from wesj:

app.getContentResolver().query(BrowserContract.Passwords.CONTENT_URI,
             null,
             "hostname = ?",
             {"chrome://weave (Mozilla Services Password)",
             null);

// alternatively
var loginmanager = Components.classes["@mozilla.org/login-manager;1"]152                .getService(Components.interfaces.nsILoginManager);
syncpasswords = loginmanager.findLogins(out  unsigned long count, in AString aHostname, in AString aActionURL, in  AString aHttpRealm, [retval, array, size_is(count)] out nsILoginInfo  logins);

The former needs the crypto stuff to land first. The latter a few hacks to treat it as a pref, and "you run the risk of throwing up the masterpassword dialog.... and its running on the gecko thread".

Given this, I'll just hold until the entirety of the Password Manger lands and pick the first, cleaner solution.
Depends on: 718760
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
Comment on attachment 595397 [details] [diff] [review]
Patch 1. WIP

Review of attachment 595397 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +1851,5 @@
>                      return;
>                  }
>  
> +                // Sync settings need Gecko to be loaded.
> +                checkMigrateSync();

Are we guaranteed that getProfileDir() will evaluate to the same value in both  checkMigrateProfile() and checkMigrateSync()?

::: mobile/android/base/ProfileMigrator.java
@@ +169,5 @@
> +        public void handleMessage(String event, JSONObject message) {
> +            Log.i(LOGTAG, "Received event: " + event);
> +            try {
> +                if (event.equals("Preferences:Data")) {
> +                    Log.i(LOGTAG, "Message: " + message.toString());

This looks like it would log sync credentials? If so, it should use the PII logger than rnewman wrote
>Are we guaranteed that getProfileDir() will evaluate to the same value in both  
>checkMigrateProfile() and checkMigrateSync()?

Yes, because we don't have any support for changing it. Not sure what this question is getting at though. Those functions need to act on the profile and ask for it. if the selected profile gets changed (pretty difficult during startup in reality), they'll look at different things. But I wouldn't know what would be reasonable behavior in that situation anyway.
Depends on: 734211
Whiteboard: [sync]
Depends on: 739629
No longer depends on: 734211
Given that bug 739629 is still missing, its very unlikely this will make the beta cut on 16/4. 

Unlike the other migration stuff, I think this is a feature that we could reasonably add during the beta, conditional on checking if the user has already configured a Sync account. If she hasn't, we wont harm her by migrating the old settings, if present, at that point. I suspect most users won't immediately set up sync (the different with getting history/bookmarks migrated!) so this gives us a little more time.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> Given that bug 739629 is still missing, its very unlikely this will make the
> beta cut on 16/4. 

I thought uplift was on the 24th?

This is very likely to be done within the next few days.
blocking-fennec1.0: beta+ → soft
Example code for use of API, yoiked from Nick's test:

import org.mozilla.gecko.sync.setup.SyncAccounts;
import org.mozilla.gecko.sync.setup.SyncAccounts.SyncAccountParameters;

...

    final String TEST_USERNAME  = "testAccount@mozilla.com";
    final String TEST_SYNCKEY   = "testSyncKey";
    final String TEST_PASSWORD  = "testPassword";
    final String TEST_SERVERURL = "testServerURL";

    // Client ID and name.
    final String TEST_NAME      = "testName";
    final String TEST_GUID      = "testGuid";

    final SyncAccountParameters params;
    params = new SyncAccountParameters(context, null,
                                       TEST_USERNAME, TEST_SYNCKEY,
                                       TEST_PASSWORD, TEST_SERVERURL, null,
                                       TEST_NAME, TEST_GUID);

    final Account account = SyncAccounts.createSyncAccount(syncAccount);
    assertNotNull(account);

Code will follow after a little more testing.
Dependency has been on inbound since this evening.

https://hg.mozilla.org/integration/mozilla-inbound/rev/190a237b1456

Over to you, gcp.
Depends on: 745540
I realize this failure is probably expected, but it is problematic in that most people who will have aurora/beta will probably also have the release installed. So this will get very little testing with the current restriction:

I/ProfileMigrator(17191): Got password
I/ProfileMigrator(17191): services.sync.account = gcp@sjeng.org
I/ProfileMigrator(17191): services.sync.client.name = gcp@sjeng.org's Fennec on GT-P7510
I/ProfileMigrator(17191): services.sync.client.GUID = v8s3Nl9LwnE5
I/ProfileMigrator(17191): Not set = services.sync.serverURL
I/ProfileMigrator(17191): services.sync.clusterURL = https://phx-sync522.services.mozilla.com/
I/ProfileMigrator(17191): services.sync.engine.addons = 0
I/ProfileMigrator(17191): services.sync.engine.prefs = 0
I/ProfileMigrator(17191): Not set = services.sync.engine.prefs.modified
I/SyncAccounts(17191): Setting explicit server URL: https://auth.services.mozilla.com/
W/AccountManagerService(  307): caller uid 10087 is different than the authenticator's uid
F/FxSync  (17191): Unable to create account. If you have more than one version of Firefox/Beta/Aurora/Nightly/Fennec installed, that's why.
F/FxSync  (17191): java.lang.SecurityException: caller uid 10087 is different than the authenticator's uid
F/FxSync  (17191): 	at android.os.Parcel.readException(Parcel.java:1321)
F/FxSync  (17191): 	at android.os.Parcel.readException(Parcel.java:1275)
F/FxSync  (17191): 	at android.accounts.IAccountManager$Stub$Proxy.addAccount(IAccountManager.java:547)
F/FxSync  (17191): 	at android.accounts.AccountManager.addAccountExplicitly(AccountManager.java:502)
F/FxSync  (17191): 	at org.mozilla.gecko.sync.setup.SyncAccounts.createSyncAccount(SyncAccounts.java:189)
F/FxSync  (17191): 	at org.mozilla.gecko.ProfileMigrator$SyncTask.configureSync(ProfileMigrator.java:437)
F/FxSync  (17191): 	at org.mozilla.gecko.ProfileMigrator$SyncTask.handleMessage(ProfileMigrator.java:351)
F/FxSync  (17191): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1759)
F/FxSync  (17191): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
F/FxSync  (17191): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
F/FxSync  (17191): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:466)
F/FxSync  (17191): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:112)
E/SyncAccounts(17191): Failed to add account Account {name=gcp@sjeng.org, type=org.mozilla.firefox_sync}!
E/ProfileMigrator(17191): Failed to create Sync account
E/GeckoAppShell(17191): handleGeckoMessage throws java.util.ConcurrentModificationException
E/GeckoAppShell(17191): java.util.ConcurrentModificationException
E/GeckoAppShell(17191): 	at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:573)
E/GeckoAppShell(17191): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1758)
E/GeckoAppShell(17191): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoAppShell(17191): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoAppShell(17191): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:466)
E/GeckoAppShell(17191): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:112)
(In reply to Gian-Carlo Pascutto (:gcp) from comment #12)
> I realize this failure is probably expected, but it is problematic in that
> most people who will have aurora/beta will probably also have the release
> installed. So this will get very little testing with the current restriction

Yup, understood. The operation doesn't make sense for users migrating multiple Fennecs; there is only one set of Sync accounts, so until we untangle that mess (and propagate the calling application and profile name) there's no sense in trying to create a second.


> org.mozilla.gecko.sync.setup.SyncAccounts.createSyncAccount(SyncAccounts.
> java:189)
> F/FxSync  (17191): 	at
> org.mozilla.gecko.ProfileMigrator$SyncTask.configureSync(ProfileMigrator.
> java:437)

Please make sure you're calling this from a background task. I presume you are, given "SyncTask" there, but CreateSyncAccountTask exists for this purpose if you're not. The AccountManager APIs will throw StrictModeExceptions on some devices if you call from the main thread.
Attachment #595397 - Attachment is obsolete: true
Attachment #615407 - Flags: review?(mark.finkle)
Attachment #615407 - Flags: feedback?(rnewman)
This appeared again briefly earlier. Should I post a log? If so what should I be looking for within the log?
Comment on attachment 615407 [details] [diff] [review]
Patch 1. Sync settings migration

Review of attachment 615407 [details] [diff] [review]:
-----------------------------------------------------------------

This seems good, modulo code nits.

::: mobile/android/base/ProfileMigrator.java
@@ +219,5 @@
> +        "services.sync.serverURL",
> +        "services.sync.clusterURL",
> +        "services.sync.engine.addons",
> +        "services.sync.engine.prefs",
> +        "services.sync.engine.prefs.modified"

I don't think you need those last three.

@@ +326,5 @@
> +        protected void requestValues() {
> +            mSyncSettingsList = Arrays.asList(kSyncSettingsList);
> +            mSyncSettingsMap = new HashMap<String, String>();
> +            JSONArray jsonPrefs = new JSONArray(mSyncSettingsList);
> +            Log.i(LOGTAG, "Sending: " + jsonPrefs.toString());

Debug.

@@ +347,5 @@
> +                                                               (GeckoEventListener)this);
> +
> +                    // Now call the password provider to fill in the rest.
> +                    for (String location: kSyncRealmList) {
> +                        Log.i(LOGTAG, "Checking: " + location);

.d not .i for all of these.

@@ +382,5 @@
> +                    cursor.getColumnIndexOrThrow(Passwords.ENCRYPTED_USERNAME);
> +                final int passCol =
> +                    cursor.getColumnIndexOrThrow(Passwords.ENCRYPTED_PASSWORD);
> +
> +                if (cursor != null && cursor.moveToFirst()) {

If you're going to check null here, you should do it before you call getColumnIndexOrThrow.

@@ +387,5 @@
> +                    String user = cursor.getString(userCol);
> +                    String pass = cursor.getString(passCol);
> +                    result = pass;
> +                } else {
> +                    Log.w(LOGTAG, "No valid password for realm = " + realm);

This isn't a warning; it won't be there if there's no Sync account.

@@ +391,5 @@
> +                    Log.w(LOGTAG, "No valid password for realm = " + realm);
> +                }
> +            } finally {
> +                if (cursor != null)
> +                    cursor.close();

You can rearrange this try..finally and the null check above to clean this up.

@@ +431,5 @@
> +            final String serverURL = mSyncSettingsMap.get("services.sync.serverURL");
> +            final String clusterURL = mSyncSettingsMap.get("services.sync.clusterURL");
> +            final String clientName = mSyncSettingsMap.get("services.sync.client.name");
> +            final String clientGuid = mSyncSettingsMap.get("services.sync.client.GUID");
> +

You might want to consider verifying that all of these _except for serverURL, clientName, clientGuid_ are non-null/non-empty. If they are, there's no point kicking off the task; just give up now.

@@ +434,5 @@
> +            final String clientGuid = mSyncSettingsMap.get("services.sync.client.GUID");
> +
> +            for (String key: kSyncSettingsList) {
> +                if (mSyncSettingsMap.get(key) != null) {
> +                    Log.i(LOGTAG, key + " = " + mSyncSettingsMap.get(key));

Don't log: includes user email addresses and server URLs.

@@ +436,5 @@
> +            for (String key: kSyncSettingsList) {
> +                if (mSyncSettingsMap.get(key) != null) {
> +                    Log.i(LOGTAG, key + " = " + mSyncSettingsMap.get(key));
> +                } else {
> +                    Log.i(LOGTAG, "Not set = " + key);

.d.

@@ +452,5 @@
> +                protected void onPostExecute(Account account) {
> +                    if (account == null) {
> +                        Log.e(LOGTAG, "Failed to create Sync account");
> +                    } else {
> +                        Log.i(LOGTAG, "Creating Sync account succeeded");

s/Creating/Migrating. Also punctuation.
Attachment #615407 - Flags: feedback?(rnewman) → feedback+
>This appeared again briefly earlier. Should I post a log? If so what should I be 
>looking for within the log?

I think you're in the wrong bug? Bug 721934 perhaps? (logs are always good)
(In reply to Gian-Carlo Pascutto (:gcp) from comment #17)
> >This appeared again briefly earlier. Should I post a log? If so what should I be 
> >looking for within the log?
> 
> I think you're in the wrong bug? Bug 721934 perhaps? (logs are always good)

You're right. Sorry and thanks.
Comment on attachment 615407 [details] [diff] [review]
Patch 1. Sync settings migration

>+        protected String getPassword(String realm) {

>+                final int userCol =
>+                    cursor.getColumnIndexOrThrow(Passwords.ENCRYPTED_USERNAME);
>+                final int passCol =
>+                    cursor.getColumnIndexOrThrow(Passwords.ENCRYPTED_PASSWORD);
>+
>+                if (cursor != null && cursor.moveToFirst()) {
>+                    String user = cursor.getString(userCol);
>+                    String pass = cursor.getString(passCol);
>+                    result = pass;

I agree with Richard about refactoring this bit of code 

>+        @Override
>+        public void run() {
>+            // XXX: Land dependent bugs first
>+            // cleanupXULLibCache();

Is there a bug number you can add here?


r+, but let's also heed Richard's comments about which prefs to grab and pre-checking before starting a migration.
Attachment #615407 - Flags: review?(mark.finkle) → review+
Misspoke here. Too many negations!

> You might want to consider verifying that all of these _except for
> serverURL, clientName, clientGuid_ are non-null/non-empty. If they are,
> there's no point kicking off the task; just give up now.

We require:

* serverURL
* account
* password
* sync key

We would like:

* client name
* client GUID

We can use, but are fine with just fetching ourselves:

* clusterURL
Richard, I may have confused you here. I checked my XUL Fennec prefs and those have a clusterURL set but not a serverURL.

Checking your code, here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/190a237b1456
it will only throw in these cases:

 if (username == null) {
 if (syncKey == null) {
 if (password == null) {
 
So I think the serverURL is *NOT* required.
Probably the default server url pref was not overridden, so you won't be able to retrieve it. If that's the case, null is fine.
Blocks: 746362
https://hg.mozilla.org/mozilla-central/rev/36c22a5ddbe5
Target Milestone: --- → Firefox 14
Flags: in-litmus?(andreea.pod)
Test case added: https://litmus.mozilla.org/show_test.cgi?id=58151
Flags: in-litmus?(andreea.pod) → in-litmus+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attached image Sync after migration
I tested this and after migration the sync settings are migrated but the bookmarks and history and the rest of the profile is not migrated so after installing the Native build it is like I have a new profile. When trying to sync I get an error saying: "Sync is currently experiencing problems. It will be back shortly." 

The exact steps that I followed:
- install the Nightly XUL build from 2012-05-06
- set up sync (the bookmarks and history were synced) 
- install over the XUL build the Native build: Firefox 15.0a1 (2012-05-06)
- check the sync

Devices:
- LG Optimus 2X (Android 2.2.2)
- Samsung Galaxy SII (Android 2.3.4)

I searched for a bug about this but couldn't find anything.
>I searched for a bug about this but couldn't find anything.

File new ones?

I think bug bug 721084 causes complete dataloss on migration, which is tracked in bug 746860. But I'm not sure, I'm just guessing because I see similar QA reports in all my migration related bugs (even though none of these issues look related to migration at all).
Blocks: 752514
Ok, thank you, bug 752492 was logged.
No longer blocks: 752514
Blocks: 752514
(In reply to Andreea Pod from comment #26)

> sync I get an error saying: "Sync is currently experiencing problems. It
> will be back shortly." 

The sync that runs after account creation is somehow getting bad credentials, leading to an authentication error, which shows like this.
You need to log in before you can comment on or make changes to this bug.