Closed Bug 396052 Opened 17 years ago Closed 16 years ago

firefox fails to start when run from a directory with non-native characters in the path

Categories

(Toolkit :: Startup and Profile System, defect, P3)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: ted, Assigned: benjamin)

References

Details

(Keywords: qawanted)

Attachments

(1 file, 3 obsolete files)

I unzipped a nightly into C:\Documents and Settings\極楽\Desktop and tried running it, and I got a messagebox titled "XULRunner", which said: "Couldn't read application.ini".
Flags: blocking1.9?
application should start :)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Benjamin, will you have time to work on this, or should we find a new assignee?
I will have time to get the easy cases fixed (it's mainly a situation of auditing callers of GetModuleFileName), but there are some cases (such as launching XULRunner apps) that aren't going to get fixed for 1.9 at all.
Priority: P1 → P3
Clarifying.  Native charsets should work ok, so Japanese characters on a Japanese Windows install should be fine.  bsmedberg says this could be a problem for Thai or other locales without a native codepage, but I'm not sure that it would actually be a problem in the default install location.
Summary: firefox fails to start when run from a directory with unicode characters in the path → firefox fails to start when run from a directory with non-native characters in the path
Why is there no second-review or superreview flag I can set? grr
Attachment #288002 - Flags: review?(sspitzer)
Attachment #288002 - Flags: review?(neil)
Comment on attachment 288002 [details] [diff] [review]
Use GetModuleFileNameW exclusively, rev. 1

r=sspitzer

Note, from bug #305039 comment #9

"as long as firefox is
installed in a folder whose name does not contain any character outside the
system default code page, you can install and *run* Japanese (Chinese, Arabic,
Hebrew, Korean) version of firefox on English (French, Russian)  Windows."

I point this out because while it appears that you could unzip into one of these directions, installing via the installer may still not work properly.

(Rob, do I have that right?)
Attachment #288002 - Flags: review?(sspitzer)
Attachment #288002 - Flags: review?(robert.bugzilla)
Attachment #288002 - Flags: review+
Comment on attachment 288002 [details] [diff] [review]
Use GetModuleFileNameW exclusively, rev. 1

LaunchChild still isn't unicode-safe, so EM restart and profile manager don't work. Annoyingly, assertions are also fatal.
Attachment #288002 - Flags: review?(neil) → review-
Neil, is there something wrong with this patch or is it just incomplete?
It seems to be just incomplete, since it does at least successfully start the profile manager.
(In reply to comment #6)
> (From update of attachment 288002 [details] [diff] [review])
> r=sspitzer
> 
> Note, from bug #305039 comment #9
> 
> "as long as firefox is
> installed in a folder whose name does not contain any character outside the
> system default code page, you can install and *run* Japanese (Chinese, Arabic,
> Hebrew, Korean) version of firefox on English (French, Russian)  Windows."
> 
> I point this out because while it appears that you could unzip into one of
> these directions, installing via the installer may still not work properly.
> 
> (Rob, do I have that right?)
You do. The UNICODE version of NSIS would resolve this.
Attachment #288002 - Flags: review?(robert.bugzilla)
This is much more invasive than the first patch: it switches to passing argv as UTF8 instead of in the native character set, so that it can represent the entire character repertoire. This involves identifying all the "in" and "out" edges and making them use special codepaths on Windows...

In addition, I just made the updater use unicode for argv and external params. It continues to use native-charset strings for paths relative to itself (that is, paths from the MAR file) because these are ASCII in the MAR file and there's no need to change them.

Note that I altered nsBrowserApp.cpp but assuming this is acceptable I will make matching changes to the other ns*App files.
Attachment #292623 - Flags: review?
Attachment #292623 - Flags: review?(ted.mielczarek)
Attachment #292623 - Flags: review?(neil)
Attachment #292623 - Flags: review?
Comment on attachment 292623 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1 

>+# Always enter a Windows program through wWinMain, even if we are a console
>+# application.
>+WIN32_EXE_LDFLAGS += -ENTRY:wWinMainCRTStartup
Won't this annoy the MinGW crowd? What's wrong with:
int WINAPI wWinMain( HINSTANCE, HINSTANCE, WCHAR* args, int )
{
  // Do the real work.
  return wmain( __argc, __wargv );
}
I don't know how mingw does entry points... are you proposing that I implement wWinMain, wmain, and main... what's the value of the extra function? We still need the convert-to-utf8 logic somewhere... I still have to explicitly specify either wmainCRTStartup or wWinMainCRTStartup so that it doesn't try to run "main" directly (at least in console mode).
After reading the patch a bit more carefully, I see that in updater.cpp you are in fact using wmain (although #if !defined(__GNUC__) which might be wrong as MinGW seems to support wmain). That means that you don't need the -ENTRY flag in its Makefile. For nsBrowserApp.cpp things are a little tricker if you're trying to keep main()... maybe you can #define main umain on Windows?
I do need the -ENTRY flag for updater, at least for MSVC release (non-console) mode. I could ifdef it to be MSVC-only, so that Mingw goes straight to the wmain entry point.
I'm still not following why updater needs a manual entry point. If you use /SUBSYSTEM:WINDOWS then [w]WinMain() called, otherwise [w]main() is called.

Things get harder for debug builds of ns*App.cpp because main() is preferred.
Well, one idea that comes to mind is this:
int main(int argc, char* argv[])
{
#ifdef XP_WIN
  argv = new char*[argc + 1];
  if (!argv)
    return 127;

  /* convert __wargv to argv */
#endif
  /* UTF-8 startup here */
#ifdef XP_WIN
  /* free argv */
#endif
  return result;
}
__wargv is only accessible if you've entered through wWinMain/wmain... it crashes if you've entered through WinMain/main.
I think you have to define UNICODE macro for __wargv to be created - if you have wWinMain/wmain this macro will be defined automatically.
In mingw or in MSVC (or both)? I could try defining UNICODE only in ns*App.cpp (we can't do it universally yet)... but I don't understand what's wrong with specifying the MSVC entry point specifically.
(In reply to comment #18)
> __wargv is only accessible if you've entered through wWinMain/wmain...

Hmm, so it is... well then, how about this:

#ifdef XP_WIN
/* class NS_ConvertUTF16ArrayToUTF8Array... */
int wmain(int argc, wchar_t* wargv[])
{
  NS_ConvertUTF16ArrayToUTF8Array argv(argc, wargv);
  if (!argv)
    return 127;
#else
int main(int argc, char* argv[])
{
#endif
I just think it's less maintainable to have to specify the entry point, but if you insist, I'd prefer you used wmainCRTStartup and abolished WinMain.
Sounds good, I'll have that up today.
wmain for the win
Attachment #288002 - Attachment is obsolete: true
Attachment #292623 - Attachment is obsolete: true
Attachment #292623 - Flags: review?(ted.mielczarek)
Attachment #292623 - Flags: review?(neil)
Comment on attachment 292963 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.1

>+#define main NS_internal_main
I thought the whole point of defining the entry point was because it defaulted to mainCRTStartup because you had a main(). You could remove the -ENTRY and convert WinMain to wWinMain and let the linker work out the entry point.

>+  // be generous... UTF16 chars can take up to 6 bytes in UTF8
Not true; a bad converter will convert a UCS32 char to 6 bytes, but that would have been two UTF16 chars anyway, so still only a 3:2 ratio.

>+  while (argc) {
>+    --argc;
>+    free(argv[argc]);
>+  }
>+
>+  delete [] argv;
Any chance of some consistency?

>+DEFINES += -DUNICODE -D_UNICODE
Are there some APIs that are used without an A or W suffix? Or if this is needed to get MinGW to accept wmain then shouldn't browser/app have it too?

>+        WCHAR *cmd = ::GetCommandLineW();
>+        rv = msgWindow.SendRequest(NS_ConvertUTF16toUTF8(cmd).get());
You need to unicodify SendRequest, otherwise it'll fail to getcwd().

>+    ok = ShellExecuteW(NULL, // no special UI window
>+		       NULL, // use default verb
>+		       exePath,
>+		       cl,
>+		       NULL, // use my current directory
>+		       SW_SHOWDEFAULT) > (HINSTANCE)32;
Tabs.
Attachment #292963 - Attachment is obsolete: true
Attachment #293007 - Flags: review?(ted.mielczarek)
Attachment #293007 - Flags: review?(neil)
The NS_internal_main definition is there mainly to ensure that it never actually is the real entry point (because that would be incorrect).

The question of WinMain is complicated because in debug mode we're a console app (which defaults to main/wmain) and in release mode we're a windows app (which defaults to WinMain/wWinMain), so to be consistent and get the correct codepath in all cases it's easier to specify a single entry point to use in both cases.
Attachment #293007 - Attachment is patch: true
Attachment #293007 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 293007 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.2

But I still think it's more maintainable to leave wWinMain in.

Hmm, I just noticed that that WindowProc's WM_COPYDATA handler will need unicodification too... does that mean RegisterClassW and CreateWindowW too, or doesn't it matter for WM_COPYDATA?
Attachment #293007 - Flags: review?(neil) → review+
The hunk -615,9 +615,9 has the only bits needed in the WindowProc. WM_COPYDATA operates on bytes, not characters, but I chose to pass arguments in UTF8 to be roughly-kinda compatible with existing clients. It would be unusual to have remoting across versions, but it can happen if an installer from a new version is trying to shut down an app from an older version. In that case the arguments will all be ASCII and there is no compatibility issue.
Thanks for pointing that out. I'd overlooked that, and the -p didn't help.
Yeah, I uploaded these directly from my .hg/patches directory... I'll use extdiff for more context next time.
Comment on attachment 293007 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.2

Are you planning on copying and pasting this set of functions to every ns*App.cpp?  That sort of sucks.  Could you find a way to share the code and let it live in toolkit/xre, even if it's something weird like #including a cpp file from there?

+int wmain(int argc, WCHAR **argv)
+{
+  char **argvConverted = new char*[argc + 1];
+  if (!argvConverted)
+    return 127;
+
+  for (int i = 0; i < __argc; ++i) {

Use argc there for consistency.

+    // SendRequest: Pass the command line via WM_COPYDATA to message window.
+    NS_IMETHOD SendRequest() {

Would this confuse us if someone launched an old firefox.exe and it remoted to a new firefox.exe?

r=me, it looks like for the majority of this, if it compiles it's correct.
Attachment #293007 - Flags: review?(ted.mielczarek) → review+
Fixed on trunk, with a shared nsWindowsWMain.cpp file which is included from the various ns*App.cpp driver files.

It is possible to confuse a mixture of old and new firefox.exe in the windows remoting code... but I consider that an edge case at best, and it will continue to work in all of the common cases (launching from a shortcut or the start menu)
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Flags: in-litmus?
Keywords: qawanted
Resolution: --- → FIXED
http://wiki.mozilla.org/MozillaQualityAssurance:bug_396052_test_plan is a stab at a test plan and outline of something for litmus.
(In reply to comment #34)
> http://wiki.mozilla.org/MozillaQualityAssurance:bug_396052_test_plan is a stab
> at a test plan and outline of something for litmus.
> 

Using above test plan and Minefield build 2007123105, I get the following results:
Basic Tests: All PASS
Install Directory Tests: FAIL on second test, remaining tests not run (see Note1)
Local File Tests: Possible PASS using Default Install Directory (see Note2)

* Note1: 
 - On the first test, I could not extract to the thai-characters (TC) directory without being prompted for a password.  I had to extract to a default directory then transfer the files over to the TC directory.
 - When running the second test, I get a "application.ini not found" error.

* Note2:
 - When running the Local File Tests using the instance of Minefield that was installed using the exe installer to the default directory, I received error pages, but the pages did attempt to load properly.
 - The first opening of a TC.html file resulted in an error page opening in Minefield
 - The second opening of a TC.html file resulted in an error page opening in a new tab
 - In both instances, the location bar URL read "file:///C:/Documents%20and%20Settings/mozilla/My%20Documents/%3F%3F%3F%3F%3F%3F%3F.html"
 - In both instances, the page error read "Firefox can't find the file at /C:/Documents and Settings/mozilla/My Documents/???????.html."
This was checked in this morning, and didn't make today's nightlies, so you'll need to try next year's builds to test it properly.  ;-)
(In reply to comment #36)
> This was checked in this morning, and didn't make today's nightlies, so you'll
> need to try next year's builds to test it properly.  ;-)
> 

I see...I will rerun my tests when I can track down the next nightly or an hourly post-730am
Ok, here are my new test results using an hourly from 10am on Dec 31st (build 2007123110):

http://wiki.mozilla.org/MozillaQualityAssurance:bug_396052_test_plan#Results_.28ashughes.29
Comment on attachment 293007 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.2

>@@ -384,20 +384,17 @@ nsCommandLine::ResolveFile(const nsAStri
>     // going to fail, and I haven't figured out a way to work around this without
>     // the PathCombine() function, which is not available in plain win95/nt4
Off topic, but this looks like a missed opportunity...
The checkin has busted the Sunbird win32 tinderbox <http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird>

d:/builds/tinderbox/Sunbird-Trunk/WINNT_5.2_Depend/mozilla/calendar/sunbird/app/nsCalendarApp.cpp(54) : fatal error C1083: Cannot open include file: 'nsWindowsMain.cpp': No such file or directory
Depends on: 410485
Version: unspecified → Trunk
Depends on: 410708
ashughes, could I get you to restest now that I've fixed bug 410610?
Depends on: 410610
(In reply to comment #42)
> ashughes, could I get you to restest now that I've fixed bug 410610?
> 

Sure, I will post my results when I complete my testing.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010811 Minefield/3.0b3pre 

VERIFIED FIXED based on bsmedberg's test case.  Please see the following wiki for the testcase and my results:

http://wiki.mozilla.org/MozillaQualityAssurance:bug_396052_test_plan#Results_.28ashughes.29_ROUND_2
Status: RESOLVED → VERIFIED
Depends on: 411826
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.