We'd like a way to pre-populate sync details from another profile to help the first run experience. If about:accounts is started with a special query string then we take some extra actions: Find the profile marked Default=1 in profiles.ini If it exists, pre-populate the details into the signup form If it does not exist, then when the sync setup is complete, show a message to the user (text to be decided) See bug 1079785 for details.

This patch brings us most of the way there: looks for the Default=1 profile and copies signedInUser.json to the current profile. This makes sync almost fully configured except for the password. Clicking on "reconnect"/"sign in" once will complete the process. It doesn't display a message if no such file is found. It also expects to be run from a different app than Firefox, so the path traversal includes an additional hop. It could be simplified further if it is to be used from the same application, while running with a different profile. It imports the MIT-licensed ini-parser library from https://github.com/isaacs/ini into the tree, for parsing profiles.ini.

Tested on Mac and Linux. The only changes to upstream ini-parser.js was incorporating the license text in the file (which seemed like a good idea) and a one-line change to use Services.appinfo.OS instead of process.platform to inquire the platform type.

Comment on attachment 8501759 [details] [diff] [review] migrate-sync Review of attachment 8501759 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +1311,5 @@ > + } > + let originalFile = OS.Path.join(stableFirefox, defaultProfile, "signedInUser.json"); > + let copiedFile = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); > + try { > + OS.File.copy(originalFile, copiedFile); My understanding is that starting in Fx34, the sync credentials will be stored in the password manager (bug 1013064).

How can we retrieve the sync credentials from the password manager in a different profile? Are they stored in a SQLite database?

Someone from the Desktop team could probably help with that. :markh?

All interaction with the login manager is via the login manager interfaces. I believe they recently started using JSON - see logins.json in your profile dir and toolkit/components/passwordmgr/LoginStore.jsm and storage-json.js for the implementation. Note a master-password is going to be a fly in the ointment (ie, you will probably need to prompt for the MP in the old profile and manually use that to decrypt the contents.) With some refactoring, it might be possible to arrange to create new login related objects and initialize then with the other profile dir. FTR, dolske is the "owner" of the login manager stuff.

Minor update that migrates from the default profile in the same application, not one in a sibling directory.

Properly handle cases where we don't guess the default profile correctly.

Patch against current gum HEAD to fix some jetpack tests.

New patch, merged all 3 current patchs related to this bug on gum: - Pre-populate sync credentials from the default Firefox profile (bug 1079835) - Don't barf on failed sync credential migrations. - the patch I just posted: Fix exception during jetpack tests

With this patch, it looks like jetpack tests become green.

Comment on attachment 8504281 [details] [diff] [review] Pre-populate sync credentials from the default Firefox profile v3 Review of attachment 8504281 [details] [diff] [review]: ----------------------------------------------------------------- Re ini-parser.js - I think we should investigate using the nsIToolkitProfileService for this. Currently it doesn't expose whether a given profile is the default, but that looks relatively easy to add - the default flag is read at http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#478, just a couple of lines after each nsToolkitProfile is created, so that flag could be passed to the constructor and exposed via nsIToolkitProfile.

(In reply to Mark Hammond [:markh] from comment #12) > Re ini-parser.js - I think we should investigate using the > nsIToolkitProfileService for this. Currently it doesn't expose whether a > given profile is the default, but that looks relatively easy to add - the > default flag is read at > http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/ > nsToolkitProfileService.cpp#478, just a couple of lines after each > nsToolkitProfile is created, so that flag could be passed to the constructor > and exposed via nsIToolkitProfile. I have a patch in bug 1024110 that exposes the default profile in the profile service (see bug 1024110 comment 16 about this). I have thought before about reusing the profiles.ini parsing logic in nsToolkitProfileService.cpp, but I've shied away from doing so because it was reaching out to another app in the previous approach and doing this in C++ would increase the risk, given the time constraints. Now that we've switched to using a profile in the same app, I'll reconsider that decision.

Another patch to improve SDK logging when a promise is rejected. Today we get a useless stack trace... I'll submit this patch to its own SDK bug, but that may help figure out our failures.

I don't get it, the issue is really fixed for me locally... Here is a blind tentative to fix it on try.

(hopefully) The tweak to jetpack unit test library helped figuring out what was wrong on try! Here is an interdiff on top of other ones. I'll shortly submit a merged'n rebased patch.

Merged, rebased patch

Here is a patch without the ini-parser library that uses the nsIToolkitProfileService defaultProfile attribute introduced by the patch in bug 1024110. I've also included Alex's fixes, my latest fix from gum, and I've moved the code from the browser startup path to a handler for the about:accounts?action=migrateToDevEdition URL. This way it should be easy to activate the feature from the First Time Experience / UI Tour code. I've left the identity.fxaccounts.migrateToDevEdition kill-switch in, as an additional guard against accidental use, but perhaps it's too much? Removing it would sure simplify the code a bit. The other thing that I'm not sure about is the end result from visiting this URL: it's either the "manage" page or the "intro" page, depending on whether migration succeeded or not. It sure looks polished and professional, but perhaps we might want to add some text somewhere to indicate what happened?

Comment on attachment 8507894 [details] [diff] [review] Pre-populate sync credentials from the default Firefox profile v5 Review of attachment 8507894 [details] [diff] [review]: ----------------------------------------------------------------- I'm missing the context of where this code is called from. ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +330,5 @@ > fxAccounts.promiseAccountsForceSigninURI().then(url => { > show("remote"); > wrapper.init(url, entryPoint); > }); > + } else if (window.location.href.contains("action=migrateToDevEdition")) { I think it makes sense to break this out into its own function. @@ +336,5 @@ > + let migrateSyncCreds = false; > + try { > + migrateSyncCreds = Services.prefs.getBoolPref("identity.fxaccounts.migrateToDevEdition"); > + } catch (e) {} > + if (migrateSyncCreds) { It looks to me that because you aren't grabbing data from the old profile's login manager, the user is going to need to reauthenticate before sync actually works. Is that the case? If so, then (a) we probably want to force that auth now and (b) we might be able to just grab the account name from signedInUser.json rather than copying the entire thing, which might be safer in the long term - that reauth will grab everything else we need.

I've made the suggested changes, except for only reading the account name from the JSON file. I don't know how to make that work. I did however avoid copying the file and just grabbed the important bits and passed them to setSignedInUser(). Now visiting that URL will lead the user right to the login screen, which eliminates all clicking. The way this feature is going to be used is from the First Time Experience / UI Tour code (see bug 1079785) that will guide the user towards setting up sync in the new profile.

Comment on attachment 8508760 [details] [diff] [review] Add ability to pre-populate sync credentials from the default Firefox profile in about:accounts v6 Review of attachment 8508760 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay and I should have mentioned this before, but I think we want tests for this - see browser_aboutAccounts. Testing this might require moving the use of the profile service into a global helper (eg, getDefaultProfilePath) which the test can stub out pointing at a directory with a test signedInUser.json. If timing really is a problem, I'd reluctantly accept a followup bug to add the tests (but given bug 1079785 has no patch yet so there's no code that takes advantage of this yet, I'd hope tests are possible) ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +387,5 @@ > + .defaultProfile; > + let fxAccountsStorage = OS.Path.join(defaultProfile.rootDir.path, fxAccountsCommon.DEFAULT_STORAGE_FILENAME); > + return OS.File.read(fxAccountsStorage, { encoding: "utf-8" }).then(text => { > + let accountData = JSON.parse(text).accountData; > + fxAccounts.setSignedInUser(accountData); I think you want to return the promise from setSignedInUser @@ +393,5 @@ > + return fxAccounts.promiseAccountsForceSigninURI().then(url => { > + show("remote"); > + wrapper.init(url, entryPoint); > + }); > + }, error => { A bit of a nit, but this error handler should be at the tail of this chain as it will not catch errors in the matching onResolve (eg, if promiseAccountsForceSigninURI failed. So it would be something like: .read().then(text => { }.then(() => { return promiseAccountsForce... ).then(null, error => { }).then(() => { // Reset the pref }); theoretically there should even be *another* .then(null, error) at the end incase the pref set fails, but that's unlikely enough we can probably live without it.

Here is the updated patch with the suggested changes. I'll try to write a test early next week.

Here is an updated version with a test. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1b215455970d

I've added some more logging to see why I'm getting a couple of oranges, but there seem to be a couple of hard to figure out races here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7cbb8ecc155 One is when I'm mocking the profile service lookup and the other is when I'm using getSignedInUser to verify that migration is finished. Would you mind landing the actual change and keep working on the test to make it more reliable? Of course any ideas you may have to that effect are more than welcome!

These are some test-only patches which should be reliable and cover what we need - they are on-top of your "v8" patch. Can you please give them a once-over and incorporate them into the main patch for landing? Try at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88b739f89173

Comment on attachment 8511957 [details] [diff] [review] v8 Review of attachment 8511957 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with requested tweaks and test changes ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +330,5 @@ > fxAccounts.promiseAccountsForceSigninURI().then(url => { > show("remote"); > wrapper.init(url, entryPoint); > }); > + } else if (window.location.href.contains("action=migrateToDevEdition")) { I should have picked this up before, but I think we should have a check for "user == null" here - if there already is a user configured we shouldn't overwrite it. @@ +336,2 @@ > } else { > // No action specified (so this comment should have appended something like ", or migration request when we already have a user") @@ +401,5 @@ > + Services.prefs.setBoolPref("identity.fxaccounts.migrateToDevEdition", false); > + }); > + } else { > + if (user) { > + show("stage", "manage"); and this block (and the 'if') can be removed, as we'd never call this function when we already have a user configured. Alternatively, keep the if and cu.reportError() in the "impossible" case. ::: browser/base/content/test/general/content_aboutAccounts.js @@ +14,5 @@ > }, true); > > addEventListener("DOMContentLoaded", function domContentLoaded(event) { > removeEventListener("DOMContentLoaded", domContentLoaded, true); > + sendAsyncMessage("test:document:domcontentloaded"); oh - the test tweaks I uploaded make this message obsolete but I forgot to remove it - can you please remove it?

Thanks for the help with the test! I made the requested changes and also removed the promiseNewTabDOMContentLoadedEvent test function, as it was no longer used after your changes. New try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e3e0329842ab

Comment on attachment 8513406 [details] [diff] [review] v9 Approval Request Comment [Feature/regressing bug #]: new feature, required for dev-edition [User impact if declined]: new dev-edition users will have to use a new separate profile and without Sync they won't have access to their profile customizations. This patch streamlines the Sync setup process and will be triggered by the UI Tour and the preferences tab/dialog. [Describe test coverage new/current, TBPL]: unit test present and manual testing on gum and fx-team [Risks and why]: minor risk as it's a fairly contained change (it is triggered only when a specific URL is visited) [String/UUID change made/needed]: none

Does this fix need manual QA coverage?

(In reply to Camelia Badau, QA [:cbadau] from comment #33) > Does this fix need manual QA coverage? This should be verified via the UI that uses it in bug 1086936 (the "Start using Sync..." link).

Verified fixed with latest gum build (buildID: 20141105175749).

I'm getting the following errors in the browser console: http://v14d.com/i/545c1ad56f0ea.jpg Which hints at this function: ``` +function getDefaultProfilePath() { + let defaultProfile = Cc["@mozilla.org/toolkit/profile-service;1"] + .getService(Ci.nsIToolkitProfileService) + .defaultProfile; + return defaultProfile.rootDir.path; ``` Build ID: 20141106040207 OS X 10.9

More importantly when I go to `about:accounts?action=migrateToDevEdition` from `about:preferences` I get a blank tab. Screenshot: http://v14d.com/i/545c1b6b48340.jpg