1024110 - Change Aurora's default profile behavior to use channel-specific profiles

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Gavin proposed this here:

https://wiki.mozilla.org/User:GavinSharp/Profile-proposal

It was posted and discussed somewhat here:

https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/c2PyBpwWmgw

I think the next step is to assign an engineer to work on this, so logging this bug.

Comment 1

[Marc Auslander]

IMHO, this is an unneeded complication.  Anyone running aurora or nightly should be able to create a shortcut with a specific profile option and a specific version of firefox.

For example, I has a shortcut to run nightly against an empty profile for verifying that problems are not caused by my customization.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

We're doing this for the people who don't want to have to learn about Firefox's command line flags or profile manager, but are still interested in running two versions side-by-side. I think that potential audience is large enough to be worth the complication.

Comment 3

[Patrick Brosset <:pbro>]

Not being "easily" able to run both release and nightly (or aurora) side by side out of the box is one of the number one complaints I've gotten from developers who use chrome daily for their work (and their one of our target audience for FirefoxDevTools). I guess this is a pretty general complaint.
This bug would help a lot!

Comment 4

[Šime Vidas]

I even documented the procedure of setting up a second profile for Nightly on the Super User forum: http://superuser.com/questions/679797/how-to-run-firefox-nightly-alongside-firefox-at-the-same-time

This is definitely something Mozilla should get rid of. Just let people install stable and Nightly side-by-side like Chrome and Canary.

Comment 6

[:Gijs (out for now; he/him)]

Gavin, is this something we could consider for the prioritized backlog, per e.g. comment #3 ? :-)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I've been trying to get the devtools team to take this unsuccessfully, but I heard it might fit into their future roadmap.

Comment 9

[Benjamin Smedberg]

Note that there are additional complexities to work out, such as what to do with remoting.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v1

I've started looking at this and here is what I've come up with so far. It's still in early stages, but one thing I'm not sure about is whether just building and running with MOZ_UPDATE_CHANNEL=aurora in my terminal is enough to simulate the behavior of an aurora binary installation.

I'm currently trying to implement Gavin's proposal without UI (just manual editing of profiles.ini) and I'll look into UI stuff later.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v2

Now with both in-content and traditional pref UI. Still WIP.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Change Aurora's default profile behavior to use channel-specific profiles

This version seems to work well and has survived all my testing so far. I think I got everything from this implemented: https://wiki.mozilla.org/User:GavinSharp/Profile-proposal

I don't know if we have tests for this sort of thing (couldn't find any), so I'm asking for review to get the ball rolling. I'll post a link to try builds shortly so you can play with them and let me know if you can spot anything wrong.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

https://tbpl.mozilla.org/?tree=Try&rev=36e11f47003d

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

I've also been thinking about the test matrix for this feature and I've jotted down my thoughts here:

https://etherpad.mozilla.org/yWUrraXqXk

I'll be happy for any feedback on that, too.

Comment 16

[Benjamin Smedberg]

Comment on attachment 8492192 [details] [diff] [review]
Change Aurora's default profile behavior to use channel-specific profiles

Is this intended to allow people to run Firefox and Aurora simultaneously?

I'm worried about the toolkit/profile usage here. The other option for this is to give Aurora an entirely separate application name, so that it has an entirely separate profiles.ini and remoting "just works". Then, to support users who want to share a profile, you can point an aurora profile name at the existing Firefox profile directory.

Some potential issues: until Firefox-release gets this change, any time Firefox release profile manager writes profiles.ini, the aurora-default will be lost. It may be worthwhile to uplift the profile-writing code to release before making the other changes in this patch.

The docs for nsIToolkitProfileService.newProfileMode are unclear, and "new" is weird in this context. Please consider an attribute name like "separateAuroraMode" so that this is clear long-term (and you'll need to rev the UUID to avoid build bustage).

Now that the default profile is stored on the nsToolkitProfile object, how do you ensure that multiple profiles don't have the default flag set on them? It looks like you could easily end up in that situation by calling nsToolkitProfile::SetDefault on multiple objects. Why is is better to store that state on the nsToolkitProfile instead of just renaming nsToolkitProfileService::mChosen to ::mDefault and adding a separate mDefaultAurora there?

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Some quick questions and answers inline until I have some time to get back at this.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> I'm worried about the toolkit/profile usage here. The other option for this
> is to give Aurora an entirely separate application name, so that it has an
> entirely separate profiles.ini and remoting "just works". Then, to support
> users who want to share a profile, you can point an aurora profile name at
> the existing Firefox profile directory.

How do you point a profile name at an existing directory? Using symlinks? If so, does that work for Windows XP? IIRC recent Windows versions have equivalent functionality, but I don't know about XP.

Also, what does "remoting" mean in this context? You had mentioned that in comment 9 as well and I want to make sure I'm not overlooking anything important.

> Some potential issues: until Firefox-release gets this change, any time
> Firefox release profile manager writes profiles.ini, the aurora-default will
> be lost. It may be worthwhile to uplift the profile-writing code to release
> before making the other changes in this patch.

Agreed.

> The docs for nsIToolkitProfileService.newProfileMode are unclear, and "new"
> is weird in this context. Please consider an attribute name like
> "separateAuroraMode" so that this is clear long-term (and you'll need to rev
> the UUID to avoid build bustage).

OK.

> Now that the default profile is stored on the nsToolkitProfile object, how
> do you ensure that multiple profiles don't have the default flag set on
> them? It looks like you could easily end up in that situation by calling
> nsToolkitProfile::SetDefault on multiple objects. Why is is better to store
> that state on the nsToolkitProfile instead of just renaming
> nsToolkitProfileService::mChosen to ::mDefault and adding a separate
> mDefaultAurora there?

It looks like mChosen has a specific purpose that this patch doesn't alter (the currently selected profile), so I guess I should add both mDefault and mDefaultAurora to nsToolkitProfileService? It does sound like a good idea of course.

Comment 18

[Marc Auslander]

I'm not knowledgeably enough to understand how this is proposed to work. So I'm just guessing.

In any case, I run the various versions by installing in the standard directory always. I run my standard profile always unless I'm testing a bug against an empty profile.  (Backups are my friend if something really messes up).

I'm hoping this won't break that.  And it leads me to think that if you want automatic different profiles for multiple installations, it should depend on the installation location, not the build version (release, aurora, nightly).

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

No, this patch won't break that workflow. Specifying a profile using -P or -profile still works as expected.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

The previous try run wasn't configured to use the aurora update channel, so testing it is useless. Sorry about that. New try:
https://tbpl.mozilla.org/?tree=Try&rev=8312f0d3f8a6

I see that I need to make some changes for Windows, apart from bsmedberg's suggestions, so I didn't request a win32 build this time.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8492192 [details] [diff] [review]
Change Aurora's default profile behavior to use channel-specific profiles

Clearing feedback and review requests until I fix the Windows build failure.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

This version takes a slightly different path per our discussions with gavin, bsmedberg and others. It uses a hard-coded profile name for aurora and a separate name (internalName) for remoting. This way aurora can run simultaneously with other Firefox versions, either can be made the default browser and opening URLs from other apps to the default app works as expected. Using an additional AuroraDefault flag in profiles.ini to select the default aurora profile among many is left as a followup due to time constraints.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8504112 [details] [diff] [review]
WIP v4

Try run only had unrelated oranges, so I'm asking for review:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aa08d33a9962

Comment 24

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8504112 [details] [diff] [review]
WIP v4

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

A quick drive-by:

* I think we should change "internal name" to something better.  "external name" seems closer to the truth, but this isn't descriptive either.  "remote name" would be an improvement IMO.

* A better long-term solution might be to write this name to profiles.ini (and thus make the name an integral part of the service).  This would allow us to have multiple profiles marked specifically as being for "firefox-dev".

In the short-term, we could assume a single profile (ie, first found with "remote=firefox-dev") is the default, but later we could have multiple profiles associated with a name.  This would mean that later we'd need a way to define a profile as being the default for a given remote-name (and probably not with "Default=1", so older Firefoxes don't get confused)

This would also leave the way open to a smarter profile-chooser UI, along the lines of Gavin's '"old profile mode"/"new profile mode"' proposal - "new profile mode" could only show profiles associated with the current remote-name.

So tl;dr - in the short-term:

* Change internal-name to remote-name.

* New nsIToolkitProfile attribute "remoteName", written as the current remote-name when a profile is created.

* Profile choosing code in nsToolkitProfileService::Init() looks for the first matching remoteName attribute rather than a name of "auroradefault".

* It creates a new profile causing the new profile to get the remoteName=firefox-dev entry.

longer term: find the devils hiding in the details, allow multiple profiles with a single remoteName, then make a smart profile UI.

::: toolkit/profile/nsIToolkitProfileService.idl
@@ +19,5 @@
>      readonly attribute nsISimpleEnumerator /*nsIToolkitProfile*/ profiles;
>  
>      attribute nsIToolkitProfile selectedProfile;
>  
> +    attribute nsIToolkitProfile defaultProfile;

Is this needed?  I can only see usage inside the profile service itself.

::: toolkit/profile/nsToolkitProfileService.cpp
@@ +487,5 @@
> +#ifdef MOZ_DEV_EDITION
> +        // Use the auroradefault profile if this is an Aurora build.
> +        if (name.EqualsLiteral("auroradefault")) {
> +            mChosen = currentProfile;
> +            this->SetDefaultProfile(currentProfile);

I'm not sure exactly how Flush() is called, but it seems like if it ends up called by this process, the SetDefaultProfile call will cause the auroradefault profile to end up with the Default=1 line?

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8504112 [details] [diff] [review]
WIP v4

Just chatted with Mark about this - the extra complication and potential loss of metadata from using old profile managers means I don't think we need to worry about his suggestions now. The "multiple profiles associated with a given 'edition'" functionality is not really a goal even in the long term.

I do agree that "internal name" is a bit of a strange name (and it's overall confusing to have basename/name/displayname/internalname etc.). "remotingName" is a better name, but only if we think it won't expand to be used for other stuff (which I think is the case?).

Similarly, shouldn't we pick a better name than "auroradefault"? If we're moving away from this being Aurora, we shouldn't leave that legacy name in there. "dev-edition-default"? Don't feel strongly, I guess we just want to avoid picking a generic enough name that people might already have a profile with that name (but even that isn't a big deal).

Shouldn't you omit the browser/confvars.sh change and only include that on Aurora where it will have a non-default value?

Nitpicking aside, I will leave the code review to Benjamin since he knows this code best, but this all looks sane to me.

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - prevent creating aurora profile on webapprt

That patch should fix tests related to webapprt.

Comment 27

[Benjamin Smedberg]

Comment on attachment 8504112 [details] [diff] [review]
WIP v4

I do agree that this should be remotingName, not internalName (also the related confvars and such).

On nsXREAppData, internalName (remotingName) should be optional. If it is null, we should fall back to `name`. You can do the null-check-and-copy at http://hg.mozilla.org/mozilla-central/annotate/77f3ca1fe052/toolkit/xre/nsAppRunner.cpp#l4130

It doesn't appear that there is any code to read RemotingName from an actual .ini file. That should be added to the table at http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/CreateAppData.cpp#110

On nsIToolkitProfileService, you need doccomments explaining the difference between .selectedProfile and .defaultProfile. It's not clear to me why we need both of them. Internally we need to keep track of them separately, but it's not clear to me why it matters externally. Going to mark r- just because of that. Ping me on IRC if we should talk about this.

Comment 28

[Jeff Griffiths (:canuckistani) (:⚡︎)]

What's the current state of this? In testing from Gum of the most recent build, it doesn't work, meanng that Dev Edition uses the single default profile on the machine I'm using to test.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jeff Griffiths (:canuckistani) from comment #28)
> What's the current state of this? In testing from Gum of the most recent
> build, it doesn't work, meanng that Dev Edition uses the single default
> profile on the machine I'm using to test.

This was caused by a gum-specific configuration change that should be fixed in my last push. Thanks for catching this!

Comment 30

[Axel Hecht [:Pike]]

I'm having two questions:

For existing users on the aurora channel, will this essentially loose all profile data?

Abstractly, I'd see potential if a distinct-profile feature would also be available to partner repacks. Less so for partners, but more so for our own variants of Firefox, if we want to create them in the future.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Axel Hecht [:Pike] from comment #30)
> I'm having two questions:
> 
> For existing users on the aurora channel, will this essentially loose all
> profile data?

No, the profile will remain intact on disk. However, they would need to use Firefox Sync to get their profile data in the new profile. I have a patch in another bug that helps with this.

Alternatively, if they so choose, they could keep using their old profile from the command line:

firefox -P oldAuroraProfile

> Abstractly, I'd see potential if a distinct-profile feature would also be
> available to partner repacks. Less so for partners, but more so for our own
> variants of Firefox, if we want to create them in the future.

I think it should be trivial to generalize this code to consult a couple of environment variables when deciding whether to use this new path and what the default profile name should be. I would replace MOZ_DEV_EDITION with, say MOZ_SPECIAL_PROFILE and introduce an additional MOZ_SPECIAL_PROFILE_NAME. I would make the latter configurable with --with-special-profile and define MOZ_SPECIAL_EDITION conditionally on the presence of MOZ_SPECIAL_PROFILE_NAME.

However, it is not a good idea to do this now, since our time constraints are very tight.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

I guess one could also remove the new dev-edition-default profile and rename their old one from the profile manager. This way they would permanently revert to what they had before.

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v5

In this version I incorporated the webapprt fix from Alex, as well as the comments from Mark, Gavin and Benjamin.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> On nsXREAppData, internalName (remotingName) should be optional. If it is
> null, we should fall back to `name`.

It was already optional at configure time, but I also made it optional at runtime per your suggestion.

> On nsIToolkitProfileService, you need doccomments explaining the difference
> between .selectedProfile and .defaultProfile. It's not clear to me why we
> need both of them. Internally we need to keep track of them separately, but
> it's not clear to me why it matters externally.

I have one good reason for exposing the defaultProfile: in bug 1079835 I need to find the default profile from dev-edition, in order to migrate the sync credentials to the new profile. In the latest patch in that bug I'm adding an ini parser library to work around the lack of this property, but markh suggested that I wouldn't need that if I could find the default profile from script using the profile service. Updating that patch is next on my TODO list.

Comment 34

[chris hofmann]

We've asked for data about the number of users over in https://bugzilla.mozilla.org/show_bug.cgi?id=1024110

I suspect that number to be quite low.  If it is, then we should really be viewing sync as part of the problem and not part of the solution for data recovery or sharing.  If the number of sync users is extremely low on aurora it might not even be worth the effort to try and find the sycn credenticals and get them shared across profiles.

Comment 35

[Benjamin Smedberg]

Comment on attachment 8507095 [details] [diff] [review]
v5

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+  if (!mAppData->remotingName)
>+    mAppData->remotingName = strdup(mAppData->name);

Use { } braces, even for single-line conditions.

This should use SetAllocatedString rather than hand-strduping. We risk mismatched allocators otherwise.

r=me with that change.

A couple questions for you to verify:
* When running the profile manager in a "normal" Firefox, does the "Use the selected profile without asking at startup" checkbox still work? Both in the positive case and the negative case?
* Shouldn't we hide that checkbox when starting developer-edition, because it will be ineffective?

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v6

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> * When running the profile manager in a "normal" Firefox, does the "Use the
> selected profile without asking at startup" checkbox still work? Both in the
> positive case and the negative case?

Yes, StartWithLastProfile is still modified by that checkbox.

> * Shouldn't we hide that checkbox when starting developer-edition, because
> it will be ineffective?

Perhaps we should, but so far I've been working with the assumption that we no longer want to make users use the profile manager for anything, if they are not already familiar with it. In a previous iteration of the patch I had added a new-profile-mode knob to the preferences dialog/tab, even though it was affecting profiles, so that it would be more discoverable by end users (no restart or command-line parameters required).

As a matter of fact, due to some last-minute additional requirements I am going to submit an additional patch on top of this one that will reintroduce the new-profile-mode (nee separate-profile-mode) as a checkbox in preferences. This way users will be able to opt-out of the separate profile behavior if they so wish.

If you feel strongly about making the StartWithLastProfile checkbox disabled/hidden in aurora when in separate-profile-mode (it will be effective as usual in normal mode), I can do it there.

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

https://tbpl.mozilla.org/?tree=Try&rev=05d6002fa68f

Comment 38

[Benjamin Smedberg]

It just seems that *if* the user is using the profile manager with dev edition, they will end up having a useless checkbox, and so we should hide it because it doesn't do anything.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

I wrote the patch to hide the checkbox in bug 1086936 and then it occurred to me that it still works even in Developer Edition: nsAppRunner, which is the only user of nsToolkitProfileService::GetStartWithLastProfile, actually calls its value "useDefault". And the purpose of useDefault is unsurprisingly to decide between using a "default profile" or displaying the profile manager to pick one.

Developer Edition overrides the definition of "default profile" in the above, to always choose a specific profile regardless of the Default=1 flag, but I didn't change the logic that handles the (arguably misnamed) startWithLastProfile flag. And I don't believe we want to: the service we strive to provide here is to hide the complexity of profile management and using a separate profile from inexperienced users, who haven't heard about -ProfileManager or -P <profile>. Experienced users should still be able to override everything we do here, either by using -P <profile>, the profile manager, or the new toggle I'm adding in bug 1086936.

Let me know if the above make sense to you.

Comment 40

[Benjamin Smedberg]

Certainly *unchecking* the checkbox will always show the profile manager. But checking it will not start DevEdition with the currently-selected profile. Right?

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

That is correct, having StartWithLastProfile=1 in profiles.ini will not affect the profile selection, in the separate-profile-mode that bug 1086936 introduces, which will be the default for DevEdition. In "old-way mode" it will continue to force profile selection to take Default=1 into account.

However, just to make sure there are no misunderstandings, if the user starts DevEdition with -ProfileManager and in the profile selection dialog picks another profile, that profile will be used for the current session only. Any new sessions that don't start via the profile manager will behave as explained above. That is still in the spirit of respecting user choice, and in particular the workflows of experienced users familiar with profiles and the profile manager.

Comment 42

[Benjamin Smedberg]

Maybe we should invert the checkbox wording then? Note that this doesn't need to block landing, it's just that the current wording is definitely incorrect in devedition when the box is checked.

Comment 43

[Panos Astithas (he/him) [:past] (please ni?)]

OK, I proceeded with landing this patch and I can tweak the checkbox label in bug 1086936. What would be the proper wording in your opinion?

https://hg.mozilla.org/integration/fx-team/rev/1416eeab79a2

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1416eeab79a2

Comment 45

[Camelia Badau [:cbadau], Desktop Test Engineering]

Please see the following scenario: 
1. Make sure that you are on a machine with 0 profiles. 
2. Launch Aurora DevEdition (latest build from the gum twig). -> the dev-edition-default profile is created (the "Allow Firefox Developer Edition and Firefox to run at the same time" option is checked)
3. Launch Beta/Release/Nightly. -> this is not opened and the following message is received: "Firefox is already running, but is not responding. The old Firefox process must be closed to open a new window". No new profile is created. 

Do you want me to fill a new bug for this problem?

Comment 46

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Camelia Badau, QA [:cbadau] from comment #45)
> Do you want me to fill a new bug for this problem?

Yes please.

Comment 47

[Camelia Badau [:cbadau], Desktop Test Engineering]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #46)
> (In reply to Camelia Badau, QA [:cbadau] from comment #45)
> > Do you want me to fill a new bug for this problem?
> 
> Yes please.

I filled bug 1090967.

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8508777 [details] [diff] [review]
v6

Approval Request Comment
[Feature/regressing bug #]: new feature for dev-edition
[User impact if declined]: dev-edition will not be able to run simultaneously as regular Firefox
[Describe test coverage new/current, TBPL]: tested on gum, m-c and fx-team
[Risks and why]: there is no doubt that this patch is not risk-free, but I believe we have stress-tested it enough and any other issues that come up can be fixed in followup bugs
[String/UUID change made/needed]: none

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5c1f21521572

Comment 50

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This was already tested, and I don't think additional QA is needed.

Comment 51

[[e-mail]]

Comment on attachment 8508777 [details] [diff] [review]
v6

>-        if (mChosen == cur) {
>+        nsCOMPtr<nsIToolkitProfile> profile;
>+        rv = this->GetDefaultProfile(getter_AddRefs(profile));
>+        if (NS_SUCCEEDED(rv) && profile == cur) {
[Why not just replace mChosen with mDefault in the test?]

Comment 52

[Eitan Adler]

This bug is causing a significant problem: when the dev edition profile is entirely deleted, and StartWithLastProfile=1 is set FirefoxDeveloperEdition does not respect the user choice: it requires starting Fx with command line flags.

Comment 53

[:Gijs (out for now; he/him)]

(In reply to Eitan Adler from comment #52)
> This bug is causing a significant problem: when the dev edition profile is
> entirely deleted, and StartWithLastProfile=1 is set FirefoxDeveloperEdition
> does not respect the user choice: it requires starting Fx with command line
> flags.

It's not clear to me what you mean by this. This is also not the best place to note this, so could you file a new bug with more details? That would be very helpful!

Comment 54

[Eitan Adler]

For reference: 1024110

Comment 55

[Eitan Adler]

For reference: I filed bug 1183152