306375 - Make it possible to restart to a different profile

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Darin Fisher]

Make it possible to restart to a different profile

I think the easiest way to solve this is to change the code that sets the
XRE_PROFILE_PATH env var to only set that variable if it is not already set.

Note to self: one concern with this approach is how it may impact profile selection.

Comment 1

[Darin Fisher]

Attachment: v1 patch

This is a pretty straightforward patch.  I verified it by entering the
following commands into the JS console:

Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment).set("XRE_PROFILE_PATH",
"/tmp/foo")
Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup).quit(0x12)


The first command changes the XRE_PROFILE_PATH environment variable to
reference a different profile directory, and the second causes Firefox to
restart.

I also verified that normal profile selection, -P, -profile, etc. all continue
to function as expected.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194489 [details] [diff] [review]
v1 patch

+  file->GetNativePath(path);

+    PR_SetEnv(expr);

nsIEnvironment maybe? That one works for unicode strings, at least
interface-wise.

Comment 3

[Darin Fisher]

XPCOM is not running when we need to set these environment variables, so
nsIEnvironment isn't an option.  As for Unicode support, nsEnvironment just
converts the Unicode string to the native filesystem encoding, which is what
GetNativePath returns anyways.  So, there isn't any advantage to using it. 
Perhaps one day nsEnvironment might be implemented on windows to use
SetEnvironmentVariableW, and then maybe it would make sense to share the code,
but again there is the "XPCOM not running" problem.

Comment 4

[Benjamin Smedberg]

Comment on attachment 194489 [details] [diff] [review]
v1 patch

-      static char kEnvVar3[MAXPATHLEN];
+      static char kEnvVar[MAXPATHLEN];
       sprintf(kEnvVar3, "XRE_BINARY_PATH=%s", gBinaryPath);

Does this build? Where is kEnvVar3 declared?

Comment 5

[Darin Fisher]

> Does this build? Where is kEnvVar3 declared?

That was a typo.  Is that the only reason for marking this review minus?

Comment 6

[Benjamin Smedberg]

yeah, r=me if you either don't rename the string or rename it consistently

Comment 7

[Darin Fisher]

Attachment: v1.1 patch

Ok, this version went in on the trunk.	I'd really like to get this in on the
1.8 branch as well.  It's very low risk for Firefox, yet essential for an
extension that is being developed.

Comment 8

[Darin Fisher]

fixed-on-trunk

Comment 9

[Asa Dotzler [:asa]]

can we get this verified on the trunk. Ben, can you give us information about
the risk here?

Comment 10

[Benjamin Smedberg]

Comment on attachment 194561 [details] [diff] [review]
v1.1 patch

actually... this does not obey the PR_GetEnv API: you must treat an empty
string the same as a null response: so the proper code would be

char *temp = PR_GetEnv("ENVVAR");
if (!temp || !*temp)
  SaveFileToEnv(...)

Comment 11

[Darin Fisher]

Attachment: followup patch

Tada!

Comment 12

[Darin Fisher]

Attachment: v1.2 patch (for the 1.8 branch)

Comment 13

[Benjamin Smedberg]

Comment on attachment 194582 [details] [diff] [review]
followup patch

Asa, this is quite safe, given a little bit of trunk testing.

Comment 14

[Mike Schroepfer]

Verify on trunk and we'll approve branch.

Comment 15

[Darin Fisher]

(In reply to comment #14)
> Verify on trunk and we'll approve branch.

I believe it works properly on the trunk.  We know of no regressions caused by
this patch, so I think it is safe to take this on the branch.

Comment 16

[Asa Dotzler [:asa]]

time is short for beta so if this is gonna make the branch, it needs to land ASAP.

Comment 17

[Darin Fisher]

fixed1.8