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

VERIFIED FIXED

Status

()

P3
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: ted, Assigned: benjamin)

Tracking

({qawanted})

Trunk
x86
Windows XP
qawanted
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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?

Comment 1

11 years ago
application should start :)
Flags: blocking1.9? → blocking1.9+

Updated

11 years ago
Priority: -- → P1

Comment 2

11 years ago
Benjamin, will you have time to work on this, or should we find a new assignee?
(Assignee)

Comment 3

11 years ago
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.

Updated

11 years ago
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
(Assignee)

Comment 5

11 years ago
Created attachment 288002 [details] [diff] [review]
Use GetModuleFileNameW exclusively, rev. 1

Why is there no second-review or superreview flag I can set? grr
Attachment #288002 - Flags: review?(sspitzer)
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 8

11 years ago
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.
(Assignee)

Comment 11

11 years ago
Created attachment 292623 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1 

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?
(Assignee)

Updated

11 years ago
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 );
}
(Assignee)

Comment 13

11 years ago
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?
(Assignee)

Comment 15

11 years ago
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;
}
(Assignee)

Comment 18

11 years ago
__wargv is only accessible if you've entered through wWinMain/wmain... it crashes if you've entered through WinMain/main.

Comment 19

11 years ago
I think you have to define UNICODE macro for __wargv to be created - if you have wWinMain/wmain this macro will be defined automatically.
(Assignee)

Comment 20

11 years ago
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.
(Assignee)

Comment 23

11 years ago
Sounds good, I'll have that up today.
(Assignee)

Comment 24

11 years ago
Created attachment 292963 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.1

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.
(Assignee)

Comment 26

11 years ago
Created attachment 293007 [details] [diff] [review]
Pass Windows arguments around as utf-8 in most places, and macroify the updater, rev. 1.2
Attachment #292963 - Attachment is obsolete: true
Attachment #293007 - Flags: review?(ted.mielczarek)
Attachment #293007 - Flags: review?(neil)
(Assignee)

Comment 27

11 years ago
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.

Updated

11 years ago
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+
(Assignee)

Comment 29

11 years ago
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.
(Assignee)

Comment 31

11 years ago
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+
(Assignee)

Comment 33

11 years ago
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
Last Resolved: 11 years ago
Flags: in-testsuite?
Flags: in-litmus?
Keywords: qawanted
Resolution: --- → FIXED
(Assignee)

Comment 34

11 years ago
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
(Assignee)

Updated

11 years ago
Duplicate of this bug: 408455
(Assignee)

Comment 42

11 years ago
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

Updated

11 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
Duplicate of this bug: 412416
You need to log in before you can comment on or make changes to this bug.