1265368 - Stop restricting Firefox refresh to the selected/default profile

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P5)

Description

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

What it says on the tin. This is blocking some of the testing stuff in bug 888624 which in turn is blocking us keeping add-ons as well. Not a dupe of bug 749700 because of bug 749700 comment 5, as I understand it.

Comment 1

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

Attachment: MozReview Request: Bug 1265368 - enable refresh to be used with the -p switch, r?MattN

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8742523 [details]
MozReview Request: Bug 1265368 - enable refresh to be used with the -p switch, r?MattN

https://reviewboard.mozilla.org/r/47253/#review45637

In summary, I think things would be simpler and more consistent if we use id/names of the source profile instead of nsIFile since the multi-profile migrators already have a sense of profiles with IDs.

::: browser/components/migration/MigrationUtils.jsm:638
(Diff revision 1)
> +   * @param [optional] aProfileToMigrate
> +   *        If set, the migration wizard will import from the profile directory
> +   *        indicated.

Why not profile name? Then we could re-use `_selectedProfile`

::: browser/components/migration/content/migration.js:224
(Diff revision 1)
>    {
>      var dataSources = document.getElementById("dataSources");
>      while (dataSources.hasChildNodes())
>        dataSources.removeChild(dataSources.firstChild);
>  
> -    var items = this._migrator.getMigrateData(this._selectedProfile, this._autoMigrate);
> +    let profileInfo = this._autoSelectedProfile || this._selectedProfile;

`_selectedProfile` is a name whereas `_autoSelectedProfile` is a dir. We should keep them both names.

::: toolkit/modules/ResetProfile.jsm:24
(Diff revision 1)
> -    let profileService = Cc["@mozilla.org/toolkit/profile-service;1"].
> -                         getService(Ci.nsIToolkitProfileService);
> -    let currentProfileDir = Services.dirsvc.get("ProfD", Ci.nsIFile);
> -
> +    // Reset is only supported if the self-migrator used for reset exists.
> +    let migrator = "@mozilla.org/profile/migrator;1?app=" + MOZ_BUILD_APP +
> +                   "&type=" + MOZ_APP_NAME;
> +    return (migrator in Cc);
> -    // Reset is only supported for the default profile if the self-migrator used for reset exists.
> -    try {
> -      return currentProfileDir.equals(profileService.selectedProfile.rootDir) &&
> -        ("@mozilla.org/profile/migrator;1?app=" + MOZ_BUILD_APP + "&type=" + MOZ_APP_NAME in Cc);
> -    } catch (e) {
> -      // Catch exception when there is no selected profile.
> -      Cu.reportError(e);
> -    }
> -    return false;
>    },

I think we should still check that the current profile is known by the toolkit profile service so this returns false for `--profile` runs. It also seems like we should support `-P` too in the C++.

::: toolkit/profile/nsIProfileMigrator.idl:27
(Diff revision 1)
> +   * Whether we're doing a profile reset.
> +   */
> +  readonly attribute boolean isProfileReset;

This still feels a little bit wrong to me since it's only relevant to one migrator but I don't see a trivial non-hacky solution so I guess this is fine.

::: toolkit/xre/ProfileReset.cpp:59
(Diff revision 1)
> -ProfileResetCleanup(nsIToolkitProfile* aOldProfile)
> +ProfileResetCleanup(nsIToolkitProfile* aOldProfile,
> +                    nsIFile* aProfileDir,
> +                    nsIFile* aProfileLocalDir)

This is another case where only supporting reset from named profiles would reduce complexity.

Comment 3

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

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2)
> Comment on attachment 8742523 [details]
> MozReview Request: Bug 1265368 - Firefox refresh shouldn't be restricted to
> the default profile, r?MattN
> 
> https://reviewboard.mozilla.org/r/47253/#review45637
> 
> In summary, I think things would be simpler and more consistent if we use
> id/names of the source profile instead of nsIFile since the multi-profile
> migrators already have a sense of profiles with IDs.

Not all profiles have a name/id - and specifically, marionette ones don't, because they're temporary ones constructed in the OS user's tmp dir. So this wouldn't solve the issue in bug 888624. With that in mind, what needs to change about this patch to make it more palatable?

Comment 4

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/47253/#review45881

(In reply to :Gijs Kruitbosch from comment #3)
> > In summary, I think things would be simpler and more consistent if we use
> > id/names of the source profile instead of nsIFile since the multi-profile
> > migrators already have a sense of profiles with IDs.
> 
> Not all profiles have a name/id

Right, I know that but since you weren't supporting -P or --profile I thought that's all your were trying to add support for in this bug. That would be my preferred solution if it weren't for…

> marionette ones don't, because they're temporary ones constructed in the OS user's tmp dir. So this
> wouldn't solve the issue in bug 888624. With that in mind, what needs to
> change about this patch to make it more palatable?

It actually would still help with bug 888624 as named profiles don't have to be in UAppData and you can name any profile:

profSvc.createProfile(Services.dirsvc.get("ProfD", Ci.nsIFile), "marionette-test-profile-" + Date.now());
profSvc.flush();

This patch would mean you wouldn't have to mess with the defaults which reminds me of an issue with your patch:

::: toolkit/xre/nsAppRunner.cpp:4185
(Diff revision 1)
>        // Set the new profile as the default after we're done cleaning up the old default.
>        rv = SetCurrentProfileAsDefault(mProfileSvc, mProfD);
>        if (NS_FAILED(rv)) NS_WARNING("Could not set current profile as the default");

This is no longer ideal as it could turn a non-default profile into a default one. (Not actually the same profile but from the user's perspective).

Given what I said above, what do you think of only supporting named profiles? It would handle the following things for you in bug 888624:

* no need to manually create a profile
* no need to restart into the new profile
* we wouldn't be changing which profile is the default (with the fix to the above)

With the above I would even say you don't need to backup profiles.ini but just use `toolkitProfile.remove(false) && toolkitProfile.flush()` at the end. Let Marionette handle the actual file deletion in the temp. dir. I just tested this approach by hand in the Browser Console and profiles.ini was identical to before naming and remove(false) on a profile in /tmp.

Thoughts?

Comment 5

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

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #4)
> > marionette ones don't, because they're temporary ones constructed in the OS user's tmp dir. So this
> > wouldn't solve the issue in bug 888624. With that in mind, what needs to
> > change about this patch to make it more palatable?
> 
> It actually would still help with bug 888624 as named profiles don't have to
> be in UAppData and you can name any profile:
> 
> profSvc.createProfile(Services.dirsvc.get("ProfD", Ci.nsIFile),
> "marionette-test-profile-" + Date.now());
> profSvc.flush();
> 
> This patch would mean you wouldn't have to mess with the defaults which
> reminds me of an issue with your patch:
> 
> ::: toolkit/xre/nsAppRunner.cpp:4185
> (Diff revision 1)
> >        // Set the new profile as the default after we're done cleaning up the old default.
> >        rv = SetCurrentProfileAsDefault(mProfileSvc, mProfD);
> >        if (NS_FAILED(rv)) NS_WARNING("Could not set current profile as the default");
> 
> This is no longer ideal as it could turn a non-default profile into a
> default one. (Not actually the same profile but from the user's perspective).
> 
> Given what I said above, what do you think of only supporting named
> profiles? It would handle the following things for you in bug 888624:
> 
> * no need to manually create a profile

But I wasn't manually creating the profile beyond doing exactly what you just said about "naming" any profile.

> * no need to restart into the new profile

Nor that... the only restart was to actually reset the browser, for which a restart is obviously necessary...

> * we wouldn't be changing which profile is the default (with the fix to the
> above)

That's the only thing that changes, which seems orthogonal to the names vs. directories thing.

Using directories rather than names would avoid the "name the profile" dance.

Comment 6

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

Per discussion with Matt, moving forward with:

- doing this per profile name. This means the test will continue to have to set the name, but this patch will be a lot smaller - this will clean up the profileresetcleanup signature;
- need to make the profile becoming the default depend on whether we were already the default;
- get rid of the special-casing for bug 1100118 and always show about:welcomeback, even when you've just imported a different profile, which means we can get rid of the idl addition and goop to pass that around;

Comment 7

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

Attachment: MozReview Request: Bug 1265368 - enable resetting non-default profiles, r?MattN

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

Comment 8

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

Attachment: MozReview Request: Bug 1265368 - ensure we flush even if we're not marking the profile as default, r?MattN

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

Comment 9

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

Comment on attachment 8742523 [details]
MozReview Request: Bug 1265368 - enable refresh to be used with the -p switch, r?MattN

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

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8749395 [details]
MozReview Request: Bug 1265368 - enable resetting non-default profiles, r?MattN

https://reviewboard.mozilla.org/r/50921/#review47591

::: toolkit/xre/nsAppRunner.cpp:2220
(Diff revision 1)
>  static nsresult
> -SetCurrentProfileAsDefault(nsIToolkitProfileService* aProfileSvc,
> +SetCurrentProfileAsSelected(nsIToolkitProfileService* aProfileSvc,
> -                           nsIFile* aCurrentProfileRoot)
> +                            nsIFile* aCurrentProfileRoot)
>  {

I think this code is actually supposed to be setting defaultProfile so I think we should hold off on renaming this until bug 1270615.

::: toolkit/xre/nsAppRunner.cpp:2570
(Diff revision 1)
> -        else
> +          } else {
> +            gResetOldProfileName.Truncate(0);
> -          gDoProfileReset = false;
> +            gDoProfileReset = false;

NS_WARNING here too?

::: toolkit/xre/nsAppRunner.cpp:4205
(Diff revision 1)
> +      if (profileWasSelected) {
> +        //XXXgijs this is actually "broken" - see bug 1270615
> +        rv = SetCurrentProfileAsSelected(mProfileSvc, mProfD);

I thought you found a method that works but I guess I misunderstood. Since you're not regressing this and it only matters to users with more than one profile before a reset I think it's fine.

I don't think the "XXXgijs" is necessary though

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8749396 [details]
MozReview Request: Bug 1265368 - ensure we flush even if we're not marking the profile as default, r?MattN

https://reviewboard.mozilla.org/r/50923/#review47603

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8742523 [details]
MozReview Request: Bug 1265368 - enable refresh to be used with the -p switch, r?MattN

https://reviewboard.mozilla.org/r/47253/#review47607

::: toolkit/xre/nsAppRunner.cpp:2481
(Diff revision 2)
> +          } else {
> +            gResetOldProfileName.Truncate(0);
> +            gDoProfileReset = false;
> +          }

Ditto about an NS_WARNING here

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5407cab3faa6
https://hg.mozilla.org/integration/fx-team/rev/e65c3c218671

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/5407cab3faa6
https://hg.mozilla.org/mozilla-central/rev/e65c3c218671