User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-KS; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080904035000 Minefield/3.1b1pre When firefox.exe is executed by start command in batch file, Software update shows RunAs dialog. And ,after inputing correct password, updater never executed. Reproducible: Always Steps to Reproduce: 1.create batchfile ---------------- @echo off set MOZ_NO_REMOTE=true start "Minefield" "C:\Program Files\Minefield\firefox.exe" -no-remote -p Trunk %1 %2 ----------- 2.create shortcut of batch file on desktop and set workind directory as "C:\" 3.double click shortcut of batch file 4.Do software update with "Help" - "Check for Update" 2. 3. Actual Results: If update exists and download completed, update dialog is shown. But, when starting update, "RunAs Dialog" is shown. But selecting any user and option, updater NEVER start update. Expected Results: Software update should not fail. When runnning firefox.exe on same user with shortcut (to C:\Program Files\Minefield\firefox.exe -no-remote -p Trunk), update never fails.

I landed a patch today in bug 453693 that likely fixes this use case. Can you try again by first updating to tomorrow's build (09-05) and check if it works with the next update after that (09-06)?

(In reply to comment #1) > I landed a patch today in bug 453693 that likely fixes this use case. > > Can you try again by first updating to tomorrow's build (09-05) and check if > it works with the next update after that (09-06)? Okay. I 'll test and report result here.

(In reply to comment #2) This issue is still reproduced. (In reply to comment #2) In some case, this issue is not reproduced by steps in comment #0 Some pre-steps is needed. 1.uninstall firefox and firefox trunk, 2.remove all profiles and firefox related file on your "Local Settings" folder and "App Config" floder 3.install trunk ,but ,you MUST NOT launch Minefield from installer 4.do step written in Comment #0

Do you get the run as dialog still?

Also, when you say, "In some case, this issue is not reproduced by steps in comment #0 Some pre-steps is needed." were you able to reproduce without the additional steps?

(In reply to comment #4) > Do you get the run as dialog still? Yes. I get run asdialog still. Update related files (update.status, update, updater.ini, updater.exe) are exists on corret directory: %USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla Firefox\updates\0 (In reply to comment #5) > Also, when you say, "In some case, this issue is not reproduced by steps in > comment #0 Some pre-steps is needed." were you able to reproduce without the > additional steps? Once update succeeded update from shortcut made by installer,I cannot reproduce this bug. To reproduce again, steps in comment #3 is needed.

We shouldn't be adding the files to %USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla Firefox\updates\0 unless you are running on Vista with UAC turned on. What does "Update related files (update.status, update, updater.ini, updater.exe) are exists on corret directory: %USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla Firefox\updates\0" mean specifically? Does it mean that you updated them into this directory or that they were updated into that directory by the update process? Have you performed any custom configuration of permissions on your system or set the bat file to launch using different permissions, etc?

I can reproduce this. When Firefox launches updater, the commandline is like "C:\path\updater.exe" "dir-path" "pid" "C:\" "C:\Program Files\Mozilla Firefox\firefox.exe" but I guess "C:\" should be "C:\\". Otherwise, double-quote will be unescaped, and argv[3] will be C:" C:\Program argv[4] will be Files\Mozilla This is not Application Update issue, but I guess problem is toolkit/xre/nsWindowsRestart.cpp.

(In reply to comment #8) > This is not Application Update issue, but I guess problem is > toolkit/xre/nsWindowsRestart.cpp. Oops, it might be wrong. It might be in NSPR.

Updater is launched from WinLaunchChild at http://hg.mozilla.org/mozilla-central/annotate/f5cd1cb9419e/toolkit/xre/nsWindowsRestart.cpp#l264 from ApplyUpdate at http://hg.mozilla.org/mozilla-central/annotate/9737aa363037/toolkit/xre/nsUpdateDriver.cpp#l477

(In reply to comment #10) > Updater is launched from > WinLaunchChild at Thanks. It's not related to NSPR.

I think, QuoteString at http://hg.mozilla.org/mozilla-central/annotate/f5cd1cb9419e/toolkit/xre/nsWindowsRestart.cpp#l90 should be fixed because it does care command-line parser of Windows. See http://msdn.microsoft.com/ja-jp/library/17w5ykft(VS.80).aspx

proposal of alternative code of QuoteString and QuotedStrLen (I'ii write patch later) static int QuoteStrLen(const PRUnichar *s) { int i = 2; // initilal and final quote int backslashCount = 0; while (*s) { if (*s == '"') { i++; i+=(backslashCount*2); backslashCount=0; } else if (*s == '\\') { backslashCount++; } else { backslashCount=0; } ++s; } i+=(backslashCount*2); } static PRUnichar* QuoteString(PRUnichar *d, const PRUnichar *s) { *d = '"'; ++d; int backslashCount = 0; while (*s) { *d = *s; if (*s == '"') { if (while (backslashCount >0) { ++d; *d = '\\'; backslashCount--; } ++d; *d = '"'; } else if (*s == '\\') { backslashCount++; } else { backslashCount=0; } ++d; ++s; } while (backslashCount >0) { ++d; *d = '\\'; backslashCount--; } *d = '"'; return d; }

(In reply to comment #13) Oops, return i is needed at tail of QuotedStrLen.

FYI: Similar quoting is already at http://hg.mozilla.org/mozilla-central/annotate/d7b1a5551e2c/xpcom/threads/nsProcessCommon.cpp#l109

This sample shows all argv and re-composed commandline string. MakeCommandLine is same as MakeCommandLine of nsWindowsRestart.cpp.

(In reply to comment #15) > FYI: > Similar quoting is already at > http://hg.mozilla.org/mozilla-central/annotate/d7b1a5551e2c/xpcom/threads/nsProcessCommon.cpp#l109 Thank you. But I found another problem. It uses char** for argv. If argv is Shift_JIS encoded, it may not work correctly.

1.Fixed String length mismatch between QuotedStrLen and QuotedString 2.QuotedString does not escape double-quotation correctly.

Comment on attachment 338116 [details] [diff] [review] patch rev.1.0 for Trunk (based on proposal rev.2) This seems like it always adds a backslash when it encounters a backslash. This will end up with C:\ -> C:\\ and when it is called again C:\\ -> C:\\\\. There is a pre-existing bug with this code where it always quotes as well. U've got a fix for the above and am just about done with tests. If you don't mind I can take this bug.

Notes: I've never liked how MakeCommandLine always adds quotes to the args and the root cause here is due to C:\ being turned into "C:\". If we pass "C:\" "C:\Program Files\" will per the MS spec become argv[1] C:" C:\Program argv[2] Files" Escaping all \ then "C:\\" "C:\\Program Files\\" becomes argv[1] C:\ argv[2] C:\\Program Files\ If we only quote args that contain a space or a tab and escape the last " when it is preceded by a \ then the commandline would be C:\ "C:\Program Files\"

I think , quoting algorithm of assembleCmdLine in nsCommonProcess.cpp is better than MakeCommandLine. But on mulibyte code handling, and safe process executing, nsWindowsRestart.cpp seems to be better (I filed bug 454608 for problems of nsCommonProcess.cpp) And, I have a question. Both of them has almostly same function. Shoule we unify them?

I agree that the quoting algorithm in assembleCmdLine is better than MakeCommandLine but neither are all that good. ;) nsIProcess is in really bad shape from what I've seen and I think it may be a good idea to unify them after nsIProcess has been improved.

Comment on attachment 338116 [details] [diff] [review] patch rev.1.0 for Trunk (based on proposal rev.2) Rob should probably review this. (if he wants this patch)

Comment on attachment 338116 [details] [diff] [review] patch rev.1.0 for Trunk (based on proposal rev.2) There are a couple of problems with the existing code this doesn't address. While writing the tests I created a patch that addresses them so I'm going to take this bug.

Comment on attachment 339918 [details] [diff] [review] patch rev1 The change to updater.cpp is to handle the case where an app wants to provide multiple command line arguments in the updater.ini.

Comment on attachment 339917 [details] [diff] [review] Tests rev 1 Found a problem so I'm going to hold off for a bit

Comment on attachment 340002 [details] [diff] [review] Tests rev 2 bah... I forgot to clean up the test's Makefile

Comment on attachment 339918 [details] [diff] [review] patch rev1 + BOOL hasDuobleQuote = wcspbrk(s, L"\"") != NULL; Typo in the variable name there. Also, any reason you're using wcspbrk instead of wcschr, given that you're only searching for one specific character? Looks good. Thanks for the informative comments!

Comment on attachment 340402 [details] [diff] [review] Tests rev 3 + // to the binary being executed and MakeCommandLine only handlkes argv[1] and typo in the comment. + wprintf(L"TEST%sFAIL | %s ARGC Comparison | Test %2d\n", passes ? L"-" : L"-KNOWN-", LOG_PREFIX, testNum); I should document this on a wiki page somewhere, but the test result values we're using are: TEST-PASS TEST-KNOWN-FAIL TEST-UNEXPECTED-FAIL TEST-UNEXPECTED-PASS The first two are not considered errors, the second two are. The first two can also be followed by (EXPECTED RANDOM), but that's only used in reftest currently. Otherwise, looks great!

Comment on attachment 340402 [details] [diff] [review] Tests rev 3 I totally +ed the wrong r?.

+ int i = wcslen(s); + BOOL hasDuobleQuote = wcspbrk(s, L"\"") != NULL; Shouldn't we have a null check on s before calling these? wcslen will exception on a null pointer.

Updated patch to Ted's and Jim's comments.

Comment on attachment 340642 [details] [diff] [review] patch rev2 Carrying forward Ted's r+

Addresses Ted's comments... carrying forward r+

Comment on attachment 340644 [details] [diff] [review] Tests rev 4 Note: I changed the existing tab in command line test and added a new tab in command line test.

Comment on attachment 340644 [details] [diff] [review] Tests rev 4 Jim, I looked at all of the callers and the null pointer check should not necessary. If a caller ever does send a null pointer we should throw an exception.

Benjamin, could you review this patch? Thanks

Comment on attachment 341155 [details] [diff] [review] patch rev3 Carrying forward r+ from Ted

Comment on attachment 341155 [details] [diff] [review] patch rev3 Boy... unit tests for the arg-quoting routines would be so nice! Any thoughts how we could accomplish that?

Benjamin, I'm not sure if you are teasing or if there is something I am missing? Attachment #340644 [details] [diff] covers the tests which include quoting arguments, etc.

I was not joking, but I was only reading the attachment I was reviewing.

Sorry about that... I didn't realize you hadn't seen the tests.

patch with tests as checked in with one minor change to protect against callers that pass in 0 args

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/14db21f85e282a63f2b1cb70fdd9d8df324515a0

Forgot to include the xre makefile change http://hg.mozilla.org/mozilla-central/rev/d391ce2f8e81bb8cfa32a16c92daf493c5d51f37

Could this have caused: https://bugzilla.mozilla.org/show_bug.cgi?id=459569 ?

Backed out the updater.cpp changes due to bug 459569. Filed bug 459615 to clean up the windows' restart code