386826 - First run of XULRunner fails on Windows Vista, showing the Usage dialog instead

Status: RESOLVED DUPLICATE of bug 437349

(Toolkit Graveyard :: XULRunner, defect)

Description

[Roland Boon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 1.9a3pre

Our application is installed in '[...]\Program Files\CompanyName ProductName Version'
The XULRunner redistributable binaries are in a subfolder called 'xulrunner'
Our application specific code is in a subfolder called 'xul'.
Application.ini is found in the 'xul' subfolder.
We have a C++ DLL and corresponding xpt file in 'xul/components'.

We have a small WIN32 application that starts xulrunner.exe, specifying the full path of application.ini in the command line.

The first run of XULRunner requires a restart after creating the user's profile. After the restart, the path to the application.ini file is no longer in the command line. That is to be expect because should be found using NS_GetEnv.
I modified xulrunner to output the content of the variable 'appDataFile' of the line

const char *appDataFile = PR_GetEnv("XUL_APP_FILE");

in nsXULRunnerApp.cpp. The value of this variable is 0. On Windows XP, it holds the path the application.ini file.


Reproducible: Always

Steps to Reproduce:
1. If already installed, uninstall the application based on XULRunner
2. Install the application based on XULRunner

Alternatively, delete the profile from both Users\User\AppData\Local and 
Users\User\AppData\Roaming.
Actual Results:  
The Usage dialog is displayed.

Expected Results:  
The application's main window appears.

The profile directories are created. I can see an .autoreg file in the users\user\AppData\Roaming profile. The contents of compreg.day and xpti.dat seems fine to me, meaning that our C++ DLL is nicely parsed.

Comment 1

[Roland Boon]

For XP_WIN, the meat of the work of NS_GetEnv() and NS_SetEnv() is done by getenv() and putenv(), respectively. I ran the two test applications based on MSDN2 documentation on _execv(). I found that changing the value of an environment variable in a process persists for its 'child' process, as expected.

Comment 2

[Roland Boon]

Starting XULRunner in a command box using the following command line does not lead to showing the Usage dialog.

xulrunner\xulrunner.exe xul\application.ini

Our application that launches XULRunner links against tinylibc. This seems to mess up the environment handling.

Comment 3

[Roland Boon]

Starting XULRunner in a command *does* lead to the showing of the Usage dialog. [Note to self: delete the profile before starting test.]

The fact the our application links against tinylibc has no relevance.

I traced the problem to WinLaunchChild() call in toolkit/xre/nsAppRunner.cpp. _execv() inherits the environment settings, WinLaunchChild() does not.

WinLaunchChild() is implemented in toolkit/xre/nsWindowsRestart.cpp.
In our case needElevation is -1, it will call LaunchAsNormalUser(). 

The user is admin at this time in the testing process.

As the code explains, CreateProcessWithTokenW does not exist on WinXP. Here is where the difference between XP and Vista starts.

Comment 4

[Roland Boon]

I isolated the WinLaunchChild() code into a separate test application. The 'Character Set' was set to 'Not Set', instead of the default 'Use Unicode Character Set'. The code worked as advertized.

I did learn that GetProcAddress(GetModuleHandle("shell32.dll"), "IsUserAnAdmin"); on Vista returned 0. According to the MSDN documentation it should, so this was a surprise. This means that the restart call will go through CreateProcess().

On XP, you can select "Run As" (right click on the executable in Windows Explorer). The dialog has a checkbox with the text "Protect my computer and data from unauthorized program activity". With that option set, the GetProcAddress() call also fails, just as on Vista. 

If you run the test application on Vista with Compatibility mode set for Windows XP (Service Pack 2), the GetProcAddress() call succeeds. The retrieval of CreateProcessWithTokenW requires Privilege Level "Run this program as an administrator".

How this combination is ever going to work is beyond me.

Comment 5

[Roland Boon]

Final note.

No restart problem occurs when changing WinLaunchChild() to _execv().

Comment 6

[Benjamin Smedberg]

Rob, can you take a look and let me know if there are any codepaths in WinLaunchChild which intentionally or accidentally might drop the current environment?

Comment 7

[Ben Bucksch (:BenB)]

Re the WinLaunchChild, I digged the source history a bit: The changes to this line are due to bug 367540, bug 351949, and bug 354226, but originally WinLaunchChild() was introduced instead of _execv() for bug 303599.

Comment 8

[Ben Bucksch (:BenB)]

Alternative would be to use the Windows Registry instead of (or in addition to) the environment variables. All we need is to store something temporarily across process boundaries.

Comment 9

[Ben Bucksch (:BenB)]

WinLaunchChild is passed gRestartArgv. Roland says this contains the commandline params for the current process, but not including -app. bsmedmerg, do you know why? It seems like the obvious fix to add that to the commandline instead of passing via env vars. (Or is it because all commandline params are ignored when there's a -app?) I can't see a good reason not to simply put it on the commandline.

Comment 10

[Benjamin Smedberg]

Because WinLaunchChild is passed the argv/argc from XRE_main, which is only the app flags. Thus the code at

http://mxr.mozilla.org/mozilla/source/xulrunner/app/nsXULRunnerApp.cpp#429 and
http://mxr.mozilla.org/mozilla/source/xulrunner/app/nsXULRunnerApp.cpp#405

Comment 11

[Ben Bucksch (:BenB)]

With "app flags", you mean *not* the --app param, and not the -install-* params, but only the custom flags that the XUL app may have?

This doesn't answer my question why you don't just re-*add* them. Take argv/argc from XRE_main as you do now, but add 2 params in front, namely "--app" and ini file path. Any reason not to do that? That would save the whole env var stuff.

Comment 12

[Benjamin Smedberg]

Because we don't know that we're in XR by that point.

Comment 13

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

(In reply to comment #6)
> Rob, can you take a look and let me know if there are any codepaths in
> WinLaunchChild which intentionally or accidentally might drop the current
> environment?
We drop privs on startup since the previous install might be 2.0.0.1 or below which will launch the app with elevated privileges since software update is elevated and we have no way of telling software update to do things differently until the next update. We also do this for the launch after install scenario but that one can be mitigated.

Comment 14

[Benjamin Smedberg]

Does dropping privileges also lose the environment, and is there any way to prevent that?

Comment 15

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

It does and I know of no standard way to prevent that.

Comment 16

[Benjamin Smedberg]

At http://mxr.mozilla.org/mozilla/source/toolkit/xre/nsWindowsRestart.cpp#217
we're passing NULL, which *documented* to inherit the environment of the calling process. Is it perhaps inheriting the environment of the shell process (the token that's being duplicated) instead?

Perhaps we could pass an explicit environment block there, by for instance calling GetEnvironmentStrings?

Comment 17

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

That sounds reasonable. The one edgecase where I think this would break is if the initial process is launch as an administrator that required the user to enter a different account which would then make the env point to the other user's data.

Comment 18

[Ben Bucksch (:BenB)]

I don't understand comment 12. I'm thinking of (pseudo-code):
@nsXULRunnerApp.cpp:272::class AutoAppData
+  nsCOMPtr<nsIFile> mAppDataFile = nsnull;
@nsXULRunnerApp.cpp:423
  AutoAppData appData(appDataLF);
+ appData.mAppDataFile = appDataLF;
@nsAppData.cpp::ScopedAppData
+ this->mAppDataFile = aAppDataFile;
@nsAppRunner.cpp::LaunchChild
+ if (gAppData && gAppData.mAppDataFile)
+ {
+    argv++; argv++; argc++; argc++;
+    argv[0] = "--app";
+    argv[1] = gAppData.mAppDataFile.path();
+ }
  #if defined(XP_MACOSX)

Comment 19

[Benjamin Smedberg]

I am a little loath to add another flag to nsXULAppInfo, but we can if we have to. If you do, please fixup the nsBrowserApp.cpp code to use the same flag, as we now have the primitive ability to run XULRunner apps from firefox.exe.

Comment 20

[Ben Bucksch (:BenB)]

I see. Any other way to pass this from nsXULRunnerApp.cpp to nsAppRunner.cpp is fine with me as well. A possible hack would be a new function SetAppDataFile(a) which just does gAppDataFile = a;, that would be an even smaller change.

Comment 21

[Ben Bucksch (:BenB)]

(It would be a function in nsAppRunner.cpp, and called by nsXULRunnerApp.cpp before calling XRE_main())

Comment 23

[Roland Boon]

In response to Ben's proposal, it is not necessary to store the AppDataFile. It is stored in the environment variable XUL_APP_FILE which is good until the restart. Moreover, NS_GetEnv returns a (const char*) string. Getting such a string from a nsILocalFile::GetPath() is more work.

Comment 24

[Roland Boon]

Followed up on Ben's suggestion and created the function XRE_SetAppDataFile() to set the variable gAppDataFile. Only in the case that the gAppDataFile has a value do we add it to the command line for the restart. 
Because the XUL_APP_FILE environment variable is only used to pass along the application.ini file to the child process, it no longer has any use, and we removed it.
The proposed change does not affect the nsBrowserApp.cpp code.
The patch will be reviewed and evaluated internally. After that, we will submit the patch here.

Comment 25

[Ben Bucksch (:BenB)]

Attachment: Fix, v3, by Roland Boon

This implements the above suggestion: Remove env var passing, instead pass app.ini filename as normal -app commandline param during restart. Pass it from xulrunnerapp to apprunner via a new function XRE_SetAppDataFile.

Comment 26

[Ben Bucksch (:BenB)]

Comment on attachment 277709 [details] [diff] [review]
Fix, v3, by Roland Boon

Patch created and tested by Roland Boon.
r=BenB

Comment 27

[Ben Bucksch (:BenB)]

Attachment: Fix, v3, as diff -uw

Comment 28

[Roland Boon]

The aforementioned patch is incorrect. Sorry. If the first parameter is the path of the executable, it needs to remain to be the first parameter.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

FYI: the potential fix for bug 395661 will collide with this patch

Comment 30

[Ben Bucksch (:BenB)]

If so, then only the patch file, not the approach, as I read it.

Comment 31

[Roland Boon]

Attachment: Fix, v4, arg[0] intact

This fix leaves first copies the first args, then inserts the application.ini arguments, and finally copies the remaining arguments, if any. It also includes the requested change for nsBrowserApp.cpp.
Tested with debug builds of xulrunner and browser on Vista (Home Premium).

Comment 32

[Roland Boon]

With regards to Finkle's comment, this patch also fixes bug 395661, since it does not copy the local path but passes along the nsILocalFile and uses GetNativePath() for the restart argument.

Note, strike 'leaves' from the previous comment.

Comment 33

[Wladimir Palant]

Comment on attachment 280569 [details] [diff] [review]
Fix, v4, arg[0] intact

> -  gRestartArgc = gArgc;
> +  gRestartArgc = gArgc + 2;

What happened to |if (gAppDataFile) {gRestartArgc += 2}|? You are now always adding the --app parameter, I don't think this is a good idea.

> +  if (gRestartArgc != gArgc) {

I still think this should be |if (gAppDataFile)|

Comment 34

[Roland Boon]

@Wladimir
Correct on both issues.

Also, [offset + i] is faulty. The reason I did not found it during testing was that none of my tests had more parameters than the application.ini path. I will send in a new patch tonight.

Comment 35

[Roland Boon]

Attachment: Fix, v5, arg[0] intact

Fixes aforementioned review comments and also corrects [offset + i] mistake.
Tested on XP and Vista

Comment 36

[Roland Boon]

TortoiseCVS sees toolkit/xre/nsXULAppAPI.h as binary. Here is the diff using our local repository.

Index: nsXULAppAPI.h
===================================================================
--- nsXULAppAPI.h	(revision 2624)
+++ nsXULAppAPI.h	(working copy)
@@ -364,4 +364,13 @@
 XRE_API(void,
         XRE_FreeAppData, (nsXREAppData *aAppData))
 
+/**
+ * Stores the  application.ini file to pass as an argument during a
+ * possible restart. It should be called before calling XRE_main().
+ *
+ * @param aINIFile The application.ini file to store.
+ */
+XRE_API(void,
+        XRE_SetAppDataFile, (nsILocalFile* aINIFile))
+
 #endif // _nsXULAppAPI_h__

Comment 37

[Wladimir Palant]

Comment on attachment 280779 [details] [diff] [review]
Fix, v5, arg[0] intact

> +    gRestartArgv[j] = gArgv[i++];

I don't understand. You are always assigning the value to the same array element, j doesn't change inside the loop. What was wrong with the previous solution? offset + i looked correct.

Comment 38

[Wladimir Palant]

Attachment: Fix, v6

I tested the previous patch and it expectedly crashed when restarting with two parameters - one of them was left uninitialized in the array. But I also found out why the original solution (|offset + i|) didn't work - i might be 1 when we enter the loop, then we will skip one entry in the array. So it seems that Roland meant to write |gRestartArgv[j++]| instead of |gRestartArgv[j]| - I changed that and everything works (tried quite a few restarts with different parameters). New patch attached.

Comment 39

[Roland Boon]

Thanks Wladimir.

Comment 40

[Petr Pytelka]

I have to confirm this bug also on Windows 2003 Server with Xulrunner (BRANCH_1_8 and HEAD).
I think this is blocking issue for any release of the xulrunner.

Do you have any estimation when this patch will be in the CVS ? It could be good to apply it to both branches.

Comment 41

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

The installer no longer launches elevated now that Bug 390366 has been fixed bu the checkin of Bug 370571. We could change software update so that a stub is launched with the current privileges that launches software update with the request for elevated privileges and then have the stub launch the app. Then the de-elevation code could be removed.

Comment 42

[Ben Bucksch (:BenB)]

> The installer no longer launches elevated now

rs, that has nothing to do with this bug, though.
Petr, we are just waiting for reviews right now.

Comment 43

[Mel Reyes]

Petr, I have to agree on the usability of this patch since 1.8 branch is still borked.

I've updated my original report from April (Bug 379265) to be 1.8 branch specific, but with the differences between Branch and Trunk it makes it impossible to apply this Trunk patch.  I left the dupe bug report and am posting now to see if the 1.8 branch will ever get addressed.

Comment 44

[Petr Pytelka]

Attachment: Patch for BRANCH_1_8

Patch for BRANCH_1_8

Comment 45

[Petr Pytelka]

I created patch for the BRANCH_1_8. It is based on the patch for the HEAD. Please can you anybody take a look on it and get it to the CVS ..., 

Thanks. Petr

Comment 46

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

petr, take a look at comment #6 and comment #13

Comment 47

[Petr Pytelka]

How is it related to my patch for the BRANCH_1_8 ?

I think that nobody will make any bigger changes in this branch so I think this could be reasonable solution without side effects.

Comment 48

[Mel Reyes]

Petr, just wanted to confirm that this patch for my 1.8 branch xulrunner builds works perfectly, thanks.

Comment 49

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

I never stated that it had anything to do with your patches and I never stated any opinion regarding whether any patch is reasonable or not. I am stating that the root cause of this bug could be resolved by using a stub to request elevation to launch software update and then launch the app with the original privileges.

Comment 50

[Ben Bucksch (:BenB)]

> I am stating that the root cause of this bug could be resolved
> by using a stub to request elevation

Not really, because it appears in other, non-installer cases as well, e.g. when restarting the app programmatically, e.g. to reflect locale language changes.

Comment 51

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

Right... and the reason that it appears in other, non-installer and non software update cases as well is that we always de-elevate because software update currently always launches the app when finished and software update is elevated... the same was true for the installer.

So by using a stub that is not elevated, launching software update from this stub with a request to elevate, and when software update is completed the stub would launch the app with the original privileges would allow us to remove the code that always de-elevates!

Comment 52

[Petr Pytelka]

The question is if anybody will write such stub for 1.8 or not - it is stable branch and I think it is not possible to make such changes.

There are no doubts that solution without privilege elevation is better for 1.9.

Comment 53

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

And I am fine with that approach and never stated I wasn't... I am just passing along information that this other approach to solving this bug is available.

Comment 54

[Wolfgang Rosenauer [:wolfiR]]

I _think_ that this patch is breaking the crashreporter's feature to restart the app if the app is called via xulrunner and not the stub.
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/nsExceptionHandler.cpp#794

Comment 55

[Wladimir Palant]

Probably not - from what I can see the crash reporter saves command line arguments in the same way it saves XUL_APP_FILE environment variable. So all this patch does is making XUL_APP_FILE saving code unnecessary, everything should keep working however.

Comment 56

[Wolfgang Rosenauer [:wolfiR]]

True, What I found though is an issue with the updater.

As all support for XUL_APP_INFO environment variable is removed from nsXULRunnerApp.cpp updates for xul apps would fail IMHO.
In update case the updater gets the callback command through the restart args and versions prior to that patch won't have the app parameter and if the updater tries to launch xulrunner w/o the app parameter it wouldn't work.

So it would be nice to keep the XUL_APP_FILE check in nsXULRunnerApp.cpp for compat reasons.

Comment 57

[Ben Bucksch (:BenB)]

I don't think that affects Firefox and Thundbird, given that they don't use that param or EXE. Does any XULrunner app actually use this updater? Is that app non-beta? I don't think we need to worry about this one-time problem.

Comment 58

[Benjamin Smedberg]

I'm pretty sure songbird uses it, as might joost.

Comment 59

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think that this will indeed bite us unless you get this in quickly. We've more or less decided to force Vista users to download a new installer for our next release (since the old version is based off the 1.8 branch and we've got a few problems with it), but after that we plan on using the updater exclusively.

Comment 60

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #57)
> I don't think that affects Firefox and Thundbird, given that they don't use
> that param or EXE. Does any XULrunner app actually use this updater?

Yes. (I'm speaking for Joost here)

> Is that app non-beta? 

What is your definition of beta? Joost is a production service even if still named Beta.

> I don't think we need to worry about this one-time problem.

Why not? Because it doesn't affect Firefox or Thunderbird?
This is a real life problem for us. (Not anymore since I fixed it in our tree)

Comment 61

[Ben Bucksch (:BenB)]

bsmedberg, can we "get this in quickly"? I think we're waiting for approval1.9. Do we even need that for this patch? How can we get it faster?

It would then affect only Joost, as far as I can see. Wolfgang, could you put the env-var reading code in as a one-off for the next release only, and remove it afterwards? Alternatively, we'd need to do that in the Moz tree, but that's more overhead.

Comment 62

[Ben Bucksch (:BenB)]

Either way, you/we'd need to re-add, and after the next release remove again, the following hunk:

diff -u nsBrowserApp.cpp
+  char *appEnv = nsnull;
+  const char *appDataFile = PR_GetEnv("XUL_APP_FILE");
+  if (appDataFile && *appDataFile) {
+    rv = XRE_GetFileFromPath(appDataFile, getter_AddRefs(appini));
+    if (NS_FAILED(rv)) {
+      Output("Invalid path found: '%s'", appDataFile);
+      return 255;
+    }
+  }
+  else if (argc > 1 && IsArg(argv[1], "app")) {
-  if (argc > 1 && IsArg(argv[1], "app")) {

Comment 63

[Reed Loden [:reed]]

Checking in browser/app/nsBrowserApp.cpp;
/cvsroot/mozilla/browser/app/nsBrowserApp.cpp,v  <--  nsBrowserApp.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.196; previous revision: 1.195
done
Checking in toolkit/xre/nsXULAppAPI.h;
/cvsroot/mozilla/toolkit/xre/nsXULAppAPI.h,v  <--  nsXULAppAPI.h
new revision: 1.24; previous revision: 1.23
done
Checking in xulrunner/app/nsXULRunnerApp.cpp;
/cvsroot/mozilla/xulrunner/app/nsXULRunnerApp.cpp,v  <--  nsXULRunnerApp.cpp
new revision: 1.35; previous revision: 1.34
done

Comment 64

[Reed Loden [:reed]]

This caused an increase in RLk on both mac and linux debug tinderboxen, so I had to back out the patch.

LEAKS BEFORE THIS PATCH:
nsStringBuffer                                    8       0.00%
TOTAL                                             8       0.00%

LEAKS AFTER THIS PATCH:
nsLocalFile                                     124          -
nsStringBuffer                                   16     100.00%
TOTAL                                           140

So, addition of an nsLocalFile leak and the nsStringBuffer leak doubled.

Comment 65

[Mark Finkle (:mfinkle) (use needinfo?)]

Perhaps the new, global gAppDataFile should use nsCOMPtr:

Change:
+nsILocalFile* gAppDataFile = nsnull;

To:
+nsCOMPtr<nsILocalFile> gAppDataFile = nsnull;

Or explicitly release the object after using it inside the | if (gAppDataFile) { | block.

Comment 66

[Mel Reyes]

I'm running into the updater issue with our emusic xulrunner app, our 1.0 release was pre-restart patch and the upgrade coming out very soon is patched.  Has anyone worked on this or should play around with trying to hack this?  (1.8 branch)

Comment 67

[Roland Boon]

Thanks for including the patch.

@Comment #64
I'm glad to see that the patch did not cause unit-test failures. :-) Regarding the extra memory leaks. The suggestion in Comment #65 is okay except that SetStrongPtr() cannot be used in that case. If I'm not mistaken, a simple assignment is enough.

-nsILocalFile* gAppDataFile = nsnull;
+nsCOMPtr<nsILocalFile> gAppDataFile;
[...]
-  SetStrongPtr(gAppDataFile, aAppDataFile);
+  gAppDataFile = aAppDataFile;

Comment 68

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #67)
> 
> -nsILocalFile* gAppDataFile = nsnull;
> +nsCOMPtr<nsILocalFile> gAppDataFile;
> [...]
> -  SetStrongPtr(gAppDataFile, aAppDataFile);
> +  gAppDataFile = aAppDataFile;
> 

Change the patch to use the above. See if it still works and we could try to re-land the patch. You could try to run the leak reporting too:
http://wiki.mozilla.org/Performance:Leak_Tools

Comment 69

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 280889 [details] [diff] [review]
Fix, v6

>+    nsCAutoString iniPath;
>+    gAppDataFile->GetNativePath(iniPath);

luser and I have both spent a lot of time making sure that app startup succeeds given paths with non-ASCII characters. Have you tested this in such a case? I would recommend #ifdef'ing for windows and using GetPath() rather than GetNativePath(). Note that you *can't* use GetPath() on linux yet because the character conversion libraries aren't initialized yet.

>+    gRestartArgv[j++] = "--app";
>+    gRestartArgv[j++] = strdup(iniPath.get());

Why not just do this here to fix the leak rather than waiting for a global nsCOMPtr to go out of scope:

  SetStrongPtr(gAppDataFile, nsnull);

Comment 70

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #69)
> >+    gRestartArgv[j++] = "--app";
> >+    gRestartArgv[j++] = strdup(iniPath.get());
> 
> Why not just do this here to fix the leak rather than waiting for a global
> nsCOMPtr to go out of scope:
> 
>   SetStrongPtr(gAppDataFile, nsnull);
> 

I think I would prefer that too. No telling when the global nsCOMPtr would get cleaned up and might not get reported anyway.

Comment 71

[(not currently active) Ted Mielczarek]

(In reply to comment #69)
> luser and I have both spent a lot of time making sure that app startup succeeds
> given paths with non-ASCII characters. Have you tested this in such a case? I
> would recommend #ifdef'ing for windows and using GetPath() rather than
> GetNativePath(). Note that you *can't* use GetPath() on linux yet because the
> character conversion libraries aren't initialized yet.

I filed bug 396052 on failing to start when run from a non-ascii dir.  Is this the same code responsible?

Comment 72

[Wladimir Palant]

(In reply to comment #69)
> luser and I have both spent a lot of time making sure that app startup succeeds
> given paths with non-ASCII characters. Have you tested this in such a case?

This code at least doesn't make things worse than they are: it doesn't matter whether we pass the path through the command line or through an environment variable. But non-ASCII characters in the command line are in fact an issue, we've run into it as well only recently.

Comment 73

[Damon Sicore (:damons)]

Comment on attachment 280889 [details] [diff] [review]
Fix, v6

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 74

[Mark Finkle (:mfinkle) (use needinfo?)]

Roland - are you going to update this patch? See comment 69

Comment 75

[Benjamin Smedberg]

I'd love for somebody to test the patch I just uploaded to bug 386379 to see if it fixes this problem as well.

Comment 76

[Daniel Veditz [:dveditz]]

Comment on attachment 283012 [details] [diff] [review]
Patch for BRANCH_1_8

Since all the nightly testers are on trunk, we require that bugs be fixed in a trunk nightly before approving for the branch.

Comment 77

[Roland Boon]

Mark, I don't want to rewrite the patch because as comment #59 states, the new installer will fix it. Also, adding the Windows #ifdef's as requested in Comment #71 will be hard to test since I don't have a Linux box.

As a workaround, our XULRunner application is started by a small wrapper application that always downgrades to non-administrator (reusing the code in nsWindowsRestart.cpp). This way, it runs correctly when started by the installer as in the case the user starts it with 'Run as Administrator'.

Comment 78

[Ben Bucksch (:BenB)]

Roland, you don't need to "rewrite" the patch. There's just a "leak" somewhere, probably the tool just complains about the global string variable that we of course never free, i.e. it's spurious. Assuming that's the case, we can either ignore it or make the tool happy by freeing the string at shutdown, or if the restart can only happen due to extension manager, then you can free it after startup. Can you please look into that? I think it really is just a free at the right place.

I believe the patch is still a good idea, whether or not the installer issue appears, because it appears in other cases as well, e.g. locale change, and it's better than having to have a workaround in our wrapper.

Comment 79

[Ben Bucksch (:BenB)]

> if the restart can only happen due to extension manager,
> then you can free it after startup

I answer that myself, with the locale change forced restart (in our app). We need to hold on the string during the whole runtime. Which means the "leak" is probably spurious.

Comment 80

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

(In reply to comment #79)
> > if the restart can only happen due to extension manager,
> > then you can free it after startup
There are other cases as well such as app upgrade where components must be re-registered. NO_EM_RESTART applies to other restarts besides the EM.

Comment 82

[Ben Bucksch (:BenB)]

Status: The above patch is fine and reviewed. The tests barked on it for a non-freed string, but I believe that was false alarm, because we need the string until app shutdown. Of course we can free it to make the test happy, but there's no leak here, I think. Nobody at TomTom currently seems to give it priority to reopen the work on it.

Comment 83

[Wladimir Palant]

Attachment: Fix, v6 (without bitrot)

Only made the patch apply on current trunk, no changes.

Comment 85

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

Bug 437349 (has a patch in progress that is almost done) should fix this bug using essentially the approach outlined in comment #41

Comment 86

[Ben Bucksch (:BenB)]

That's good. But even then, I think this patch improves matters. It's good when we don't have to rely on environment vars and remove code paths, unifying on commandline arguments.

We just need somebody to make a simple change to the patch. I don't have a Windows build environment.

Comment 87

[Wladimir Palant]

Resolving as WONTFIX - we decided that using the same approach as Firefox should be a better thing to do for us.

Comment 88

[Benjamin Smedberg]

Actually let's mark this as a dup, it's certainly not WONTFIX.