1122124 - FF 35 "refresh" installer makes me reselect my profiles

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Pomax]

I have FF and FF developer edition, with FF dev (finally) using a dedicated profile so I don't have to bother selecting which to use where. The new FF35 "refresh" installation however decided it was better to forget all of that, invent a new, third, empty profile, and then ask which profile I want to use. That's not a nice experience when all I wanted was to updated FF and get back to interacting on the web.

Comment 1

[Robert Strong (they/them - no direct email)]

"Refresh Firefox" is built into Firefox and has nothing to do with the installer.

Comment 2

[Pomax]

noted? But that doesn't sound like information relevant to the report, which is about "refresh firefox" deciding to invent new profiles instead of maintaining the ones that were already in use right before I accepted to "refresh" firefox

Comment 3

[Justin Dolske [:Dolske]]

Do you remember how you triggered the profile reset? Bug 1126559 could be involved (we offer it from the website in cases when we shouldn't). If you triggered it via about:support, that's not involved though.

I suspect it's likely there are some bugs with this, as the DevEdition has to do some awkward things to support the shared / non-shared profile distinction, and profile reset may not be sufficiently aware of that (and does it's own weird things to figure if it should make the function available, and may not name the resulting profile correctly for the aforementioned DevEdition stuff).

dcamp: Maybe someone on devtools can help with this if it's a priority for DevEdition? Given its popularity with normal Firefox, could increasingly become an issue if it's not working right.

Comment 4

[Matthew N. [:MattN]]

If we are currently allow a refresh in DevEdition then we need to at least handle the fact that profile created after a reset get the prefix "default-" and so it will no longer have the name dev-edition-default.

Comment 6

[:Gijs (he/him)]

Mike: thanks for filing, and sorry this got dropped on the floor. The good news is, we're working on tests for this feature, which hopefully means it will start dealing better with some of this. Hopefully this will be fixed soon.


From the dupe I just filed:

STR:

1. open Firefox with profile manager (-P)
2. create a profile
3. start with that profile
4. open about:support
5. click 'refresh firefox', and click through the dialogs that follow, restore your tabs when asked.
6. exit Firefox

ER:
the newly-created default-yada-yada profile is selected if you now start the profile manager (it's the default) and has the Default=1 annotation in profiles.ini

AR:
neither of the above is actually the case

Panos, it seems that this got broken with the split between selectedprofile and defaultprofile when implementing a separate profile for devedition. It also seems like Fx reset/refresh code is currently the only consumer of the setselectedprofile method on the profile manager ( https://dxr.mozilla.org/mozilla-central/search?q=setselectedprofile&=mozilla-central&redirect=true ) though I guess there's some stuff in the profile manager itself as well ( https://dxr.mozilla.org/mozilla-central/search?q=%22selectedProfile%20%3D&=mozilla-central&redirect=true ).

Can you clarify how this is supposed to work?

Comment 7

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

Oh, this is bad indeed. So in bug 1024110 I introduced defaultProfile for Developer Edition, because up until then selectedProfile was used to indicate both concepts. I modified the profile selection code in the startup path and the profile manager to handle that distinction, but I did not know anything about profile reset or that it creates a new profile, so I didn't update that part. I think that SetCurrentProfileAsDefault should call SetDefaultProfile instead of or in addition to SetSelectedProfile, in order for Default=1 to be properly written to profiles.ini (I assume this is the main problem).

However Matt raises another important issue in comment 4 that I'm not sure how to fix. How does the reset feature name profiles in future invocations? default-XXX-N, default-default-XXX or something else? The name dev-edition-default is hard-coded in nsToolkitProfileService.cpp to avoid the additional complexity from having to deal with a DevEditionDefault=1 flag. What should probably happen is to make nsToolkitProfileService::Init anticipate the presence of the default-dev-edition-default (and presumably default-default-dev-edition-default, etc.) profiles and treat them with higher precedence than dev-edition-default. Or we could just bite the bullet and introduce DevEditionDefault=1, which will require changes in more places.

Comment 8

[:Gijs (he/him)]

(In reply to Panos Astithas [:past] from comment #7)
> Oh, this is bad indeed. So in bug 1024110 I introduced defaultProfile for
> Developer Edition, because up until then selectedProfile was used to
> indicate both concepts. I modified the profile selection code in the startup
> path and the profile manager to handle that distinction, but I did not know
> anything about profile reset or that it creates a new profile, so I didn't
> update that part. I think that SetCurrentProfileAsDefault should call
> SetDefaultProfile instead of or in addition to SetSelectedProfile, in order
> for Default=1 to be properly written to profiles.ini (I assume this is the
> main problem).

So... at risk of being/sounding stupid: what is the "other" concept that selectedprofile represents? It isn't the currently running profile (which e.g. when you use --profile, is different from selectedProfile), and selecting a different profile from the profile manager sets default=1 on that profile, even if 'always use this profile on startup' is unticked... so what is it?

> However Matt raises another important issue in comment 4 that I'm not sure
> how to fix. How does the reset feature name profiles in future invocations?
> default-XXX-N, default-default-XXX or something else? The name
> dev-edition-default is hard-coded in nsToolkitProfileService.cpp to avoid
> the additional complexity from having to deal with a DevEditionDefault=1
> flag. What should probably happen is to make nsToolkitProfileService::Init
> anticipate the presence of the default-dev-edition-default (and presumably
> default-default-dev-edition-default, etc.) profiles and treat them with
> higher precedence than dev-edition-default. Or we could just bite the bullet
> and introduce DevEditionDefault=1, which will require changes in more places.

I think we should probably just change the reset procedure to restore the profile name to whatever it was before; that'd fix this issue.

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> So... at risk of being/sounding stupid: what is the "other" concept that
> selectedprofile represents? It isn't the currently running profile (which
> e.g. when you use --profile, is different from selectedProfile), and
> selecting a different profile from the profile manager sets default=1 on
> that profile, even if 'always use this profile on startup' is unticked... so
> what is it?

It should very much be the profile currently (or about to be) used:

https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/toolkit/profile/nsIToolkitProfileService.idl#22

I think I remember -P being somewhat "special", but I'll have to read the code again to remember in what way. I also remember some lingering bugs with the profile manager in Developer Edition, which may be related, but my understanding at the time was that we wanted to kill it more than fix it.

> I think we should probably just change the reset procedure to restore the
> profile name to whatever it was before; that'd fix this issue.

Oh, if that is an option, absolutely!

Comment 10

[:Gijs (he/him)]

(In reply to Panos Astithas [:past] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > So... at risk of being/sounding stupid: what is the "other" concept that
> > selectedprofile represents? It isn't the currently running profile (which
> > e.g. when you use --profile, is different from selectedProfile), and
> > selecting a different profile from the profile manager sets default=1 on
> > that profile, even if 'always use this profile on startup' is unticked... so
> > what is it?
> 
> It should very much be the profile currently (or about to be) used:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> e5a10bc7dac4ee2453d8319165c1f6578203eac7/toolkit/profile/
> nsIToolkitProfileService.idl#22
> 
> I think I remember -P being somewhat "special", but I'll have to read the
> code again to remember in what way. I also remember some lingering bugs with
> the profile manager in Developer Edition, which may be related, but my
> understanding at the time was that we wanted to kill it more than fix it.

STR:

1. open Firefox from ./mach run, which uses the scratch profile
2. evaluate Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.nsIToolkitProfileService).selectedProfile.name in the browser console

ER:
exception because selectedProfile should be null because the scratch profile isn't in profiles.ini

AR:
"default" (or whatever was the last profile you ran with that had a name)

> > I think we should probably just change the reset procedure to restore the
> > profile name to whatever it was before; that'd fix this issue.
> 
> Oh, if that is an option, absolutely!

I mean, in addition to fixing this... we'll need to use SetDefaultProfile instead of SetSelectedProfile, AIUI.

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1122124 - fix default profile setting after reset, keep profile name,

Review commit: https://reviewboard.mozilla.org/r/51597/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51597/

Comment 12

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Panos Astithas [:past] from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > So... at risk of being/sounding stupid: what is the "other" concept that
> > > selectedprofile represents? It isn't the currently running profile (which
> > > e.g. when you use --profile, is different from selectedProfile), and
> > > selecting a different profile from the profile manager sets default=1 on
> > > that profile, even if 'always use this profile on startup' is unticked... so
> > > what is it?
> > 
> > It should very much be the profile currently (or about to be) used:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > e5a10bc7dac4ee2453d8319165c1f6578203eac7/toolkit/profile/
> > nsIToolkitProfileService.idl#22
> > 
> > I think I remember -P being somewhat "special", but I'll have to read the
> > code again to remember in what way. I also remember some lingering bugs with
> > the profile manager in Developer Edition, which may be related, but my
> > understanding at the time was that we wanted to kill it more than fix it.
> 
> STR:
> 
> 1. open Firefox from ./mach run, which uses the scratch profile
> 2. evaluate
> Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.
> nsIToolkitProfileService).selectedProfile.name in the browser console
> 
> ER:
> exception because selectedProfile should be null because the scratch profile
> isn't in profiles.ini
> 
> AR:
> "default" (or whatever was the last profile you ran with that had a name)

Where are we fixing the code so we get the ER above? GetCurrentProfile is just a workaround for selectedProfile being wrong which adds to the confusion.

Comment 13

[Matthew N. [:MattN]]

Note that you're mostly fixing bug 883627 here

Comment 14

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> Note that you're mostly fixing bug 883627 here

Well, that and the issue in comment #0, which has to be fixed this way irrespective of whether we change the semantics of selectedProfile.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #12)
> Where are we fixing the code so we get the ER above? GetCurrentProfile is
> just a workaround for selectedProfile being wrong which adds to the
> confusion.

I'm not. It seems to me like people are likely to be depending on selectedProfile doing what it currently does, and changing its meaning seems unlikely to produce good results. Adding a third thing ("currentProfile", or "runningProfile" or whatever) instead seems even less likely to help with the confusion. What approach would you like us to take?

Comment 15

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> > Note that you're mostly fixing bug 883627 here
> 
> Well, that and the issue in comment #0, which has to be fixed this way
> irrespective of whether we change the semantics of selectedProfile.
> 
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #12)
> > Where are we fixing the code so we get the ER above? GetCurrentProfile is
> > just a workaround for selectedProfile being wrong which adds to the
> > confusion.
> 
> I'm not. It seems to me like people are likely to be depending on
> selectedProfile doing what it currently does, and changing its meaning seems
> unlikely to produce good results.

Who do you think depends on this and how many people would be affected? I don't think usage of this API is that high and I suspect some consumers currently have incorrect data because they don't realize the shortcomings of the API as it is. I'm not worried about consumers not expecting null as that would only happen in rare cases (e.g. with --profile).

67 files on AMO - 12 (r2d2b2g) - 4 (Firefox OS 1.2 Simulator) - 30 in a comment duplicated in many extensions = 21 files possibly affected on AMO

> Adding a third thing ("currentProfile", or
> "runningProfile" or whatever) instead seems even less likely to help with
> the confusion. What approach would you like us to take?

Yeah, I don't think we should add a third thing when it's not even clear we need two.

Comment 16

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> > > Note that you're mostly fixing bug 883627 here
> > 
> > Well, that and the issue in comment #0, which has to be fixed this way
> > irrespective of whether we change the semantics of selectedProfile.
> > 
> > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #12)
> > > Where are we fixing the code so we get the ER above? GetCurrentProfile is
> > > just a workaround for selectedProfile being wrong which adds to the
> > > confusion.
> > 
> > I'm not. It seems to me like people are likely to be depending on
> > selectedProfile doing what it currently does, and changing its meaning seems
> > unlikely to produce good results.
> 
> Who do you think depends on this and how many people would be affected? I
> don't think usage of this API is that high and I suspect some consumers
> currently have incorrect data because they don't realize the shortcomings of
> the API as it is. I'm not worried about consumers not expecting null as that
> would only happen in rare cases (e.g. with --profile).
> 
> 67 files on AMO - 12 (r2d2b2g) - 4 (Firefox OS 1.2 Simulator) - 30 in a
> comment duplicated in many extensions = 21 files possibly affected on AMO

I'm less worried about external consumers and more worried about our own usecases *reading* selectedProfile and making decisions based on what it says, and what the consequences would be of effectively changing the meaning of selectedProfile in some circumstances (and/or having it throw or return null when it doesn't currently).

In particular, it is currently backed by mChosen here: https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#602 .

Which AFAICT is really only set here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#501

7 lines later for devedition only, and in the SetSelectedProfile method. The profile dialog writes this here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/content/profileSelection.js#105

But it doesn't seem like it ever gets set to something else if you start with --profile or -P .


Reading consumers seem to basically be:

https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/browser/components/nsBrowserGlue.js#1292-1299

and about:profiles and the profile manager dialog. I don't know that esp. the latter two will be happy about us changing the meaning. The nsBrowserGlue code looks like it'll throw if the profile is null. Not sure if I missed any other consumers. :-\

Really, in general, it feels like we could fix this but all the changes in this patch are still going to be necessary to fix the issues raised here and bug 1267575 / bug 883627 .

Can we move the "make selectedProfile 'sane'" project to a separate bug? :-)

Comment 17

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch (gone until June 6) from comment #16)
> Really, in general, it feels like we could fix this but all the changes in
> this patch are still going to be necessary to fix the issues raised here and
> bug 1267575 / bug 883627 .

I haven't had a chance to check this but know that I would have already reviewed a fix for bug 883627 alone. Can we do that first in order to move forward?

Comment 18

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #17)
> (In reply to :Gijs Kruitbosch (gone until June 6) from comment #16)
> > Really, in general, it feels like we could fix this but all the changes in
> > this patch are still going to be necessary to fix the issues raised here and
> > bug 1267575 / bug 883627 .
> 
> I haven't had a chance to check this but know that I would have already
> reviewed a fix for bug 883627 alone. Can we do that first in order to move
> forward?

This is actually not trivial to extract from the patch as-is, because in order to change the name of the profile to fix 883627, we need a reference to the profile, which we don't have. I could push that into SetCurrentProfileAsDefault but then it'd only work if we were setting the new profile as the default so we'd only fix it some of the time, which also seems stupid. The alternative, which is to rewrite SetCurrentProfileAsDefault to just return the current profile and push the responsibility for setting it as default or changing its name into the consumer is what the patch already does.

IOW "just" fixing bug 883627 already requires either picking this option or changing the selectedProfile semantics, or only fixing it for cases where the profile is the default.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8750706 [details]
Bug 1122124 - fix default profile setting after reset, keep profile name,

https://reviewboard.mozilla.org/r/51597/#review98258

We should probably do something about the selectedProfile attribute.

It may also be nice to use the profile name for the directory too (e.g. UX-NNNNNN) in bug 883627

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a82b89786b41
fix default profile setting after reset, keep profile name, r=MattN

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/1627991f3c62 for https://treeherder.mozilla.org/logviewer.html#?job_id=7785141&repo=autoland

Comment 22

[:Gijs (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #21)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/1627991f3c62
> for
> https://treeherder.mozilla.org/logviewer.html#?job_id=7785141&repo=autoland

Delightful, I ended up writing a test for this inbetween when I wrote the patch and when it landed, so now I need to update the test.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8750706 [details]
Bug 1122124 - fix default profile setting after reset, keep profile name,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51597/diff/1-2/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19b8210a8cce
fix default profile setting after reset, keep profile name, r=MattN

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/19b8210a8cce

Comment 26

[Azmina]

I have reproduced this bug with Nightly 38.0a1 (2015-01-15)on Windows 8.1 (64 bit!)

This bug's fix is verified on Latest Nightly 53.0a1 

Build ID : 20161219030207
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

Comment 27

[:Gijs (he/him)]

Thanks!