771766 - help->about update changes the case of the install folder

Status: RESOLVED INCOMPLETE

(Toolkit :: Application Update, defect)

Description

[al_9x]

If Fx is installed into a mixed case folder (Firefox), but the command line used to start it is all lower case (firefox\firefox.exe), update initiated through help about will lower case the install folder (firefox).

This is a 16.0 regression.

Comment 1

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

Brian / Ehsan, any idea how this could happen?

Comment 2

[(no longer active)]

GetInstallationDir which retuns the newDir path names when performing replaces uses gDestinationPath which is filled here from argv[2] <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2168>.  That itself comes from the GRE dir as far as I can tell: <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3383>.  As far as I know, that comes from the nsIFile object which stores the GRE directory in the appdata.  I don't know what kind of normalizations, if any, the local file code does on Windows.

Comment 3

[(no longer active)]

Note that the fix should be fairly simple here (merely calling GetLongPathName on newDir, as I believe that gives you the correct case stored on the file system).

Comment 4

[(no longer active)]

(Robert, please let me know if you want me or Brian to work on this.)

Comment 5

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

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> (Robert, please let me know if you want me or Brian to work on this.)
Ehsan, if you could that would be awesome! Brian is busy working on Firefox on Metro and I'm starting up stub installer work.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

The patch is quite straightforward.  I also tested locally to make sure that it works correctly.

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9740009f67ae

Comment 8

[(no longer active)]

Comment on attachment 640424 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 307181
User impact if declined: See comment 0.  This is not a very bad breakage, but it would probably confuse some people.  Also, it would probably create problems in the very rare case that somebody actually uses a case sensitive filesystem under Windows.
Testing completed (on m-c, etc.): locally, testing is trivial
Risk to taking this patch (and alternatives if risky): This patch has very minimal risk.  We already call GetLongPathName a bunch of times in the updater, and it works well in those other places.
String or UUID changes made by this patch: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9740009f67ae

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 640424 [details] [diff] [review]
Patch (v1)

[Triage Comment]
No reason to find out if this could cause user impact when we've got a low risk correctness fix. Approved for Aurora 15.

Comment 11

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/156b820c1337

Comment 12

[al_9x]

not fixed in 17.0, just did an update from yesterday's nightly to today's, folder was lower cased

Comment 13

[(no longer active)]

Hmm, which operating system are you on?  I need help from QA to see if they can reproduce this.

Comment 14

[al_9x]

xp

Comment 15

[u279076]

I am not able to reproduce this bug:
1. Install Firefox 17.0a1 2012-07-16 to "C:\Program Files\Nightly\firefox.exe"
2. Start Firefox via desktop icon to generate a profile
3. Quit Firefox and run cmd.exe
4. Run "c:\program files\nightly\firefox.exe"
5. Go to Help > About and let the update download
6. Click "Restart to Update"
7. Quit Firefox after it restarts
> Install path is still "C:\Program Files\Nightly"

Comment 16

[al_9x]

try making your install folder "Firefox 17.0"

Comment 17

[u279076]

(In reply to al_9x from comment #16)
> try making your install folder "Firefox 17.0"

Can you please elaborate? Should I be renaming "C:\Program Files\Nightly" to "C:\Program Files\Firefox 17.0" via Explorer? Should I do it with the Firefox installer? Please be more specific in your comments.

Comment 18

[al_9x]

I was not using the installer, just extract the zip and rename firefox to "Firefox 17.0"

Comment 19

[u279076]

Can you please give me detailed steps to reproduce like I did in comment 15? I need to be able to reproduce what you are seeing exactly.

Comment 20

[al_9x]

follow your step but name the folder "Firefox 17.0"

Comment 21

[u279076]

(In reply to al_9x from comment #20)
> follow your step but name the folder "Firefox 17.0"

I don't find this all that helpful. I'm not sure why you can't be more forthcoming about this, but I digress. Would the following be appropriate steps to reproduce?

1. Download the Firefox 17.0a1 2012-07-16 win32 zip and extract it to "C:\Program Files\Firefox 17.0\firefox.exe"
2. Start Firefox via desktop icon to generate a profile
3. Quit Firefox and run cmd.exe
4. Run "c:\program files\firefox 17.0\firefox.exe"
5. Go to Help > About and let the update download
6. Click "Restart to Update"
7. Quit Firefox after it restarts
8. Verify the Firefox folder is still "C:\Program Files\Firefox 17.0\"

Comment 22

[al_9x]

you going to keep whining, or go for it?  I told you three times to try "Firefox 17.0" as the install folder.

Comment 23

[u279076]

It's crucial for me to understand your steps to reproduce so that I know I'm following the same path. I'm sorry, but I cannot help you any further until you:
a) confirm my steps in comment 21 are exactly the same as yours
- or -
b) provide me with your exact detailed steps to reproduce this bug

Comment 24

[al_9x]

I told you already to follow your steps but use "Firefox 17.0" as the install folder.  That's your confirmation.

@ehsan - bugs don't go away just because psycho qa refuse to repro them

Comment 25

[(mostly gone) XtC4UaLL [:xtc4uall]]

Must be really kind of Windows XP specific then since on Windows 7 x64 I failed to repro the Issue updating Builds of 

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-07-03-04-15-19-mozilla-central/firefox-16.0a1.en-US.win32.zip

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-07-17-03-05-55-mozilla-central/firefox-17.0a1.en-US.win32.zip

*each* against the current Nightly both using "Firefox 16" resp. "Firefox 16.0" as well as  "Firefox 17" resp. "Firefox 17.0" as Folder Names.

I couldn't check if I can repro Comment 0 against Windows 7 since I don't know how to run a (manual) Update against a non-recent/custom dated Nightly.

Comment 26

[u279076]

(In reply to al_9x from comment #24)
> I told you already to follow your steps but use "Firefox 17.0" as the
> install folder.  That's your confirmation.

Since you seem unwilling to simply answer my request I will have to assume you mean my steps in comment 21 are correct. Having run those steps I do not reproduce what you are reporting.

> @ehsan - bugs don't go away just because psycho qa refuse to repro them

I've done nothing to warrant this personal attack. I'm merely trying to develop a test which does not get invalidated because of incorrect assumptions. I would appreciate you refraining from this behaviour in the future. I'm only trying to help.

Comment 27

[(no longer active)]

[e-mail], I'm going to close this bug again since neither me, Anthony or XtC4UaLL can reproduce this.  FWIW, we all tried to rename the folder to "Firefox 17.0" and that did not make a difference (and I don't see why it should, as my patch here should not fail to work for some of the folder names but work on others.)  If you have reliable steps to reproduce, please file another bug so that we can discuss this there.

Also, the second paragraph of comment 24 is way out of line.  Anthony is trying to help here, like everyone else, and this kind of behavior is not appreciated nor tolerated on this forum.  Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html.

Comment 28

[al_9x]

This bug is trivially reproducible on xp (including a clean xp sp3 vm).  The active ingredients are xp, "Firefrox 17.0" install folder, and lower case start command, that's all it takes, and all this information was given in this bug report.

The parent folder, whether you launch from "start->run" or cmd.exe, which profile you use (default or -profile), none of it matters.  That's why I don't believe any of you have actually bothered to do it, this would not have been closed otherwise.  Anthony was not trying to help, he was whining pedantically and refusing to repro (on xp with with "Firefox 17.0"), now I have to waste my time because he can't be bothered to do his job.

1. Start a clean xp sp3 vm
2. create a folder "c:\tmp"
3. download

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-07-19-03-05-43-mozilla-central/firefox-17.0a1.en-US.win32.zip

to c:\tmp
4. extract firefox-17.0a1.en-US.win32.zip to c:\tmp
5. in c:\tmp rename firefox to "Firefox 17.0"
6. start run "c:\tmp\firefox 17.0\firefox.exe" -profile c:\tmp\profile
7. help->about update, restart
8. the "Firefox 17.0" folder is changed to "firefox 17.0"

Comment 29

[u279076]

I've now tested this under three environments:
 * my original Win XP SP3 VM
 * a brand new Win XP SP3 VM
 * a PC running Win XP SP3

I've used your steps in comment 28 and retested my steps in comment 15 and 21 a few times. Here are my results:

 * Firefox 16.0a1 2012-07-07 reproduces this bug (as you reported).
 * Firefox 16.0a1 2012-07-12 does not reproduce this bug.
 * Firefox 17.0a1 2012-07-16 does not reproduce this bug.
 * Firefox 17.0a1 2012-07-19 does not reproduce this bug.

I don't think there is much more I can do in terms of testing this; there has to be something else going on. Can you please file a new bug specific to your Firefox 17 case (as asked by Ehsan in comment 27)? This is a separate yet similar issue which will need to be investigate in a separate bug report.

Thank you.

Comment 30

[al_9x]

I don't need to create another bug, the one I reported, this one, is not fixed.

firefox.exe starts updater.exe with the following command line:

Command line:	C:\DOCUME~1\tmp\LOCALS~1\Temp\MozUpdater-3\updater.exe "C:\Documents and Settings\tmp\Local Settings\Application Data\Mozilla\Firefox\firefox\updates\0" "c:\tmp\firefox 17.0\updated" 672/replace "C:\tmp\firefox 17.0" "c:\tmp\firefox 17.0\firefox.exe"
Current directory:	C:\tmp\firefox 17.0\

and here's last-update.log:

Performing a replace request
SOURCE DIRECTORY C:\Documents and Settings\tmp\Local Settings\Application Data\Mozilla\Firefox\firefox\updates\0
DESTINATION DIRECTORY c:\tmp\firefox 17.0\updated
Begin moving sourceDir (c:\tmp\firefox 17.0) to tmpDir (c:\tmp\firefox 17.0.bak)
rename_file: proceeding to rename the directory
Begin moving newDir (c:\tmp\firefox 17.0.bak/updated) to sourceDir (c:\tmp\firefox 17.0)
rename_file: proceeding to rename the directory
Now, remove the tmpDir
succeeded
calling QuitProgressUI

all "firefox 17.0" references are lower case.

Comment 31

[u279076]

I'm not seeing that either. The case is being preserved properly in my log file. Can you do an extra verification please...

As given by comment 9:
Firefox 16.0a1 2012-07-10 should exhibit this bug, Firefox 16.0a1 2012-07-11 should not.

As given by comment 11:
Firefox 15.0a2 2012-07-13 should exhibit this bug, Firefox 15.0a2 2012-07-14 should not.

Based on the merge:
Firefox 16.0a1 2012-07-16 is the last Firefox 16 mozilla-central.
Firefox 17.0a1 2012-07-17 is the first Firefox 17 mozilla-central.
Neither should exhibit this bug.

On my end, the above is the exact behaviour that I experience.

Comment 32

[al_9x]

@Ehsan - GetLongPathName does not work as you expect on XP.  It will correct the case only if the folder name length is <= 8
_________________________________________

int _tmain(int argc, _TCHAR* argv[])
{
	if (argc == 1) return 0;
	TCHAR long_path[MAX_PATH];
	if (GetLongPathName(argv[1], long_path, MAX_PATH) != 0)
		_tprintf(long_path);
	return 0;
}
_______________________________________________________

C:\Documents and Settings\tmp\Desktop>mkdir A2345678

C:\Documents and Settings\tmp\Desktop>mkdir A23456789

C:\Documents and Settings\tmp\Desktop>PathTest a2345678
A2345678
C:\Documents and Settings\tmp\Desktop>PathTest a23456789
a23456789
_______________________________________________________

And here's a thread about it:

http://groups.google.com/group/microsoft.public.win32.programmer.kernel/browse_thread/thread/2ab0de6c211b5d3a
_______________________________________________________

How would you describe Anthony Hughes' contribution to this bug?  I stand by my assessment.

Comment 33

[Brian R. Bondy [:bbondy]]

Maybe SHGetFileInfoW w/ SHGFI_DISPLAYNAME will work more consistently.

Comment 34

[al_9x]

(In reply to Brian R. Bondy [:bbondy] from comment #33)
> Maybe SHGetFileInfoW w/ SHGFI_DISPLAYNAME will work more consistently.

SHGFI_DISPLAYNAME returns the leaf folder name in the correct case, if combined with the rest of the path, the case of which is should not matter since it's not being created, it should work.

Comment 35

[(no longer active)]

What file system are you using on your Windows XP machine?  Maybe that would describe why neither I or Anthony has been able to reproduce this bug.

Also, again, the last paragraph of comment 32 is out of line, not necessary and most definitely not appreciated here.  Personal attacks on this forum are not welcome.  Please stop making these types of comments.

Comment 36

[(no longer active)]

(In reply to Brian R. Bondy [:bbondy] from comment #33)
> Maybe SHGetFileInfoW w/ SHGFI_DISPLAYNAME will work more consistently.

Hmm, I think I would prefer calling GetShortPathName followed by GetLongPathName rather than trying to extract the leaf name etc.

Comment 37

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #35)
> What file system are you using on your Windows XP machine?  Maybe that would
> describe why neither I or Anthony has been able to reproduce this bug.

NTFS.  GetLongPathName in a clean, stock xp sp3 works as I described.  I have a clean vm that I personally created and snapshotted after install.  There is no change in a fully patched one, my physical machine.  What exactly do you claim to be running?


(In reply to Ehsan Akhgari [:ehsan] from comment #36)
> (In reply to Brian R. Bondy [:bbondy] from comment #33)
> > Maybe SHGetFileInfoW w/ SHGFI_DISPLAYNAME will work more consistently.
> 
> Hmm, I think I would prefer calling GetShortPathName followed by
> GetLongPathName rather than trying to extract the leaf name etc.

Short path names can be disabled (http://support.microsoft.com/kb/121007), in which case GetShortPathName may fail (have not tested)

Comment 38

[(no longer active)]

(In reply to al_9x from comment #37)
> (In reply to Ehsan Akhgari [:ehsan] from comment #35)
> > What file system are you using on your Windows XP machine?  Maybe that would
> > describe why neither I or Anthony has been able to reproduce this bug.
> 
> NTFS.  GetLongPathName in a clean, stock xp sp3 works as I described.  I
> have a clean vm that I personally created and snapshotted after install. 
> There is no change in a fully patched one, my physical machine.  What
> exactly do you claim to be running?

I have a WinXP SP3 VM with many (all?) of the updates having been applied.

So I just tried the program in comment 32 and I did reproduce the problem with that!  But I still have not been able to reproduce this bug in Firefox!  I can't quite explain this...

Comment 39

[Brian R. Bondy [:bbondy]]

I wonder if it is some kind of cache thing? Maybe try killing explorer.exe or restarting to see if the case changes after that?

Comment 40

[(no longer active)]

(In reply to comment #39)
> I wonder if it is some kind of cache thing? Maybe try killing explorer.exe or
> restarting to see if the case changes after that?

Nope tried that, it doesn't help.

Comment 41

[al_9x]

what's your last-update.log

Comment 42

[(no longer active)]

(In reply to al_9x from comment #41)
> what's your last-update.log

Performing a replace request
SOURCE DIRECTORY C:\Documents and Settings\Ehsan\Local Settings\Application Data\Mozilla\Firefox\firefox\updates\0
DESTINATION DIRECTORY C:\Documents and Settings\Ehsan\Desktop\Firefox 17.0\updated
Begin moving sourceDir (C:\Documents and Settings\Ehsan\Desktop\Firefox 17.0) to tmpDir (C:\Documents and Settings\Ehsan\Desktop\Firefox 17.0.bak)
rename_file: proceeding to rename the directory
Begin moving newDir (C:\Documents and Settings\Ehsan\Desktop\Firefox 17.0.bak/updated) to sourceDir (C:\Documents and Settings\Ehsan\Desktop\Firefox 17.0)
rename_file: proceeding to rename the directory
Now, remove the tmpDir
succeeded
calling QuitProgressUI

Comment 43

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #42)
> (In reply to al_9x from comment #41)
> > what's your last-update.log
> 
> Performing a replace request
> SOURCE DIRECTORY C:\Documents and Settings\Ehsan\Local Settings\Application
> Data\Mozilla\Firefox\firefox\updates\0
> DESTINATION DIRECTORY C:\Documents and Settings\Ehsan\Desktop\Firefox
> 17.0\updated

Since the introduced GetLongPathName can not do what's expected of it here, the only explanation is that installDir already has the right case. "DESTINATION DIRECTORY" log line with the right case is indicative of that.

Comment 44

[(no longer active)]

(In reply to comment #43)
> (In reply to Ehsan Akhgari [:ehsan] from comment #42)
> > (In reply to al_9x from comment #41)
> > > what's your last-update.log
> > 
> > Performing a replace request
> > SOURCE DIRECTORY C:\Documents and Settings\Ehsan\Local Settings\Application
> > Data\Mozilla\Firefox\firefox\updates\0
> > DESTINATION DIRECTORY C:\Documents and Settings\Ehsan\Desktop\Firefox
> > 17.0\updated
> 
> Since the introduced GetLongPathName can not do what's expected of it here, the
> only explanation is that installDir already has the right case. "DESTINATION
> DIRECTORY" log line with the right case is indicative of that.

What do you mean by "can not do what's expected of it here"?  the case of the DESTINATION DIRECTORY log line is exactly what I would expect it to be.  Also, I launched firefox using a command line this: ".\Firefox 17.0\firefox.exe".  Is that not enough to reproduce the bug?

Comment 45

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> (In reply to comment #43)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #42)
> > > (In reply to al_9x from comment #41)
> > > > what's your last-update.log
> > > 
> > > Performing a replace request
> > > SOURCE DIRECTORY C:\Documents and Settings\Ehsan\Local Settings\Application
> > > Data\Mozilla\Firefox\firefox\updates\0
> > > DESTINATION DIRECTORY C:\Documents and Settings\Ehsan\Desktop\Firefox
> > > 17.0\updated
> > 
> > Since the introduced GetLongPathName can not do what's expected of it here, the
> > only explanation is that installDir already has the right case. "DESTINATION
> > DIRECTORY" log line with the right case is indicative of that.
> 
> What do you mean by "can not do what's expected of it here"?  the case of
> the DESTINATION DIRECTORY log line is exactly what I would expect it to be. 
> Also, I launched firefox using a command line this: ".\Firefox
> 17.0\firefox.exe".  Is that not enough to reproduce the bug?

Argh, meant to type ".\firefox 17.0\firefox.exe"!

Comment 46

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> (In reply to comment #43)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #42)
> > > (In reply to al_9x from comment #41)
> > > > what's your last-update.log
> > > 
> > > Performing a replace request
> > > SOURCE DIRECTORY C:\Documents and Settings\Ehsan\Local Settings\Application
> > > Data\Mozilla\Firefox\firefox\updates\0
> > > DESTINATION DIRECTORY C:\Documents and Settings\Ehsan\Desktop\Firefox
> > > 17.0\updated
> > 
> > Since the introduced GetLongPathName can not do what's expected of it here, the
> > only explanation is that installDir already has the right case. "DESTINATION
> > DIRECTORY" log line with the right case is indicative of that.
> 
> What do you mean by "can not do what's expected of it here"?

GetLongPathName can not turn "firefox 17.0" into "Firefox 17.0" on xp, something you already know.

> the case of
> the DESTINATION DIRECTORY log line is exactly what I would expect it to be.
> Also, I launched firefox using a command line this: ".\Firefox
> 17.0\firefox.exe".  Is that not enough to reproduce the bug?

Why do you expect DESTINATION DIRECTORY to have correct case?  If it does, there is no need to correct anything. DESTINATION DIRECTORY does not have the right case for me (Comment 30), hence the bug.

I will post a procmon log later, that will show exactly what processes are started with what command line after clicking "restart to update"

Comment 47

[al_9x]

Attachment: procmon log of processes and command lines triggered by "restart to update"

Comment 48

[(no longer active)]

Somebody who can reproduce this should take a look at it.

Comment 49

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #48)
> Somebody who can reproduce this should take a look at it.

Your "fix" is a noop on xp with folders over 8 chars.  If you don't want to bother investigating why the case is already correct for you, by the time you new code runs, but not guaranteed to be correct (since it's not for me), then at least add case correction that will actually work.  Why would you leave code that you know is broken?

Comment 50

[(no longer active)]

(In reply to comment #49)
> (In reply to Ehsan Akhgari [:ehsan] from comment #48)
> > Somebody who can reproduce this should take a look at it.
> 
> Your "fix" is a noop on xp with folders over 8 chars.  If you don't want to
> bother investigating why the case is already correct for you, by the time you
> new code runs, but not guaranteed to be correct (since it's not for me), then
> at least add case correction that will actually work.  Why would you leave code
> that you know is broken?

I do not appreciate your tone here.  Using phrases such as "don't want to bother investigating" is totally inappropriate, especially after I've been trying to reproduce the problem and fix it in the past few days.

And without being able to reproduce this bug, I can't do anything to fix it, because I can't test my work.

Also, this issue is an edge case which only happens to some XP users who run Firefox from the command line including the directory name with an incorrect case.  I'm sorry that this bug still affects you, but I will not have enough time to investigate this further.  If you or someone else wants to step up and take this over, that would be amazing!

Comment 51

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #50)
> (In reply to comment #49)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #48)
> > > Somebody who can reproduce this should take a look at it.
> > 
> > Your "fix" is a noop on xp with folders over 8 chars.  If you don't want to
> > bother investigating why the case is already correct for you, by the time you
> > new code runs, but not guaranteed to be correct (since it's not for me), then
> > at least add case correction that will actually work.  Why would you leave code
> > that you know is broken?
> 
> I do not appreciate your tone here.  Using phrases such as "don't want to
> bother investigating" is totally inappropriate, especially after I've been
> trying to reproduce the problem and fix it in the past few days.

Your tender feelings notwithstanding, that's exactly what you're doing, not bothering to investigate.  There's is something flaky going on with how firefox launches updater.exe, ever since a certain point (perhaps Bug 307181).  On my clean xp vm it always (given a lower case initial command) launches with the wrong case.  But evidently for you it always launches with the right case, cause it's not your patch in updater.exe that's fixing the case (on xp).

This flakiness should be investigated, to that end, I posted my procmon log, which you can compare with yours to see where the problem starts and review that code.  But you don't want to bother. 

> 
> And without being able to reproduce this bug, I can't do anything to fix it,
> because I can't test my work.

You don't seem to get it, the specific patch that you added to "fix" this bug, does not work on xp.  Even if you won't investigate why the case is correct to begin with, for you, but not for me (i.e. not guaranteed to be correct), you should still fix your patch to actually do (on xp) what you intended it to do, fix the case when it's not already correct.  It's completely bizarre to knowingly leave broken code. This you can definitely unit test.

Comment 52

[(no longer active)]

(In reply to comment #51)
> (In reply to Ehsan Akhgari [:ehsan] from comment #50)
> > (In reply to comment #49)
> > > (In reply to Ehsan Akhgari [:ehsan] from comment #48)
> > > > Somebody who can reproduce this should take a look at it.
> > > 
> > > Your "fix" is a noop on xp with folders over 8 chars.  If you don't want to
> > > bother investigating why the case is already correct for you, by the time you
> > > new code runs, but not guaranteed to be correct (since it's not for me), then
> > > at least add case correction that will actually work.  Why would you leave code
> > > that you know is broken?
> > 
> > I do not appreciate your tone here.  Using phrases such as "don't want to
> > bother investigating" is totally inappropriate, especially after I've been
> > trying to reproduce the problem and fix it in the past few days.
> 
> Your tender feelings notwithstanding, that's exactly what you're doing, not
> bothering to investigate.  There's is something flaky going on with how firefox
> launches updater.exe, ever since a certain point (perhaps Bug 307181).  On my
> clean xp vm it always (given a lower case initial command) launches with the
> wrong case.  But evidently for you it always launches with the right case,
> cause it's not your patch in updater.exe that's fixing the case (on xp).
> 
> This flakiness should be investigated, to that end, I posted my procmon log,
> which you can compare with yours to see where the problem starts and review
> that code.  But you don't want to bother. 

There is no flakiness going on.  I know exactly what command line is used to launch updater.exe both before and after bug 307181, as I was the person who fixed that bug.  It's just that with bug 307181, we rename the staged directory to be the new installation directory, and because we use the same arguments used to launch Firefox in order to construct the command line passed to updater.exe, the Windows API call used to perform the rename sees the lower-case version of the name, which is why the name of the folder changes.

The patch that I landed in this bug fixes this problem at least on Windows 7 (and I verified that when I was writing and testing the patch.)  And for whatever reason, it also fixes the bug on my Windows XP VM, but not yours.  And that is evident as I cannot reproduce the bug there but you can.

> > And without being able to reproduce this bug, I can't do anything to fix it,
> > because I can't test my work.
> 
> You don't seem to get it, the specific patch that you added to "fix" this bug,
> does not work on xp.  Even if you won't investigate why the case is correct to
> begin with, for you, but not for me (i.e. not guaranteed to be correct), you
> should still fix your patch to actually do (on xp) what you intended it to do,
> fix the case when it's not already correct.  It's completely bizarre to
> knowingly leave broken code. This you can definitely unit test.

Trust me, I get it.  This problem still exists on some Windows configurations, which is why the bug is not marked as FIXED yet.  And I understand that this issue is frustrating to you, and I understand that, but this needs help from somebody who _can_ reproduce the problem so that they can try to fix it.  I'm not that person.

Also to put things into perspective, read the last paragraph of comment 50.  No one is saying that this is not a problem any more.  I'm just saying that it is enough of an edge case that I'm personally comfortable with leaving it unresolved until we have more information on why this happens.  I have tons of other things that I'm working on, and this is not high priority for me, especially since I'm stuck on how to reproduce it.  Now this seems unacceptable to you, which is fine.  This is how people ship software.  No software ever released has all of the known bugs fixed.  We look at the list of known issues, prioritize them, and try to fix as many of them as possible before we ship software.

Also note that having an aggressive attitude towards the people who are working with the goal of fixing a bug never helps.  You have been behaving in that way first with Anthony and now with me.  I'm sorry, but this is not the right forum for this kind of verbal abuse.  (Not that I personally get hurt by these types of comments, but I do take the liberty of pointing out abusive behavior and ask people to stop it.)  As flaming fires never helps to put them off, I'll refrain from commenting further on this bug unless someone provides new information which can be used to fix this bug.  I hope that you cool down and that we can work together again in the future.  :-)

Comment 53

[al_9x]

> 
> The patch that I landed in this bug fixes this problem at least on Windows 7
> (and I verified that when I was writing and testing the patch.)  And for
> whatever reason, it also fixes the bug on my Windows XP VM, but not yours.

This is exactly what you are not getting.  I am doing by best to explain it to you, this would be the third or fourth time.

Your patch does not fix the problem on your xp.  GetLongPathName DOES NOT WORK on xp for "Firefox 17" (length > 8).  It does not work on my xp, your xp, any xp.  You have already confirmed this, it's settled.

The reason you can't repro is that for you, installDir already has the right case, so your patch does not need to work.  It is worth investigating why installDir sometimes has the right case and sometimes not, but this bug can be fixed universally without this investigation, by simply utilizing a case correction technique that is known to work everywhere.  One was suggested by Brian R. Bondy (in comment 33).

Knowing that your patch does not work on XP, knowing that you can easily make it work, but deliberately leaving it broken, is not justifiable.  XP remains a supported platform and no code should be checked in that is known not to work on XP.

Comment 54

[al_9x]

LPTSTR CorrectFolderCase(LPCTSTR path, LPTSTR corrected) {
	SHFILEINFO sfi;
	if (!SHGetFileInfo(path, 0, &sfi, sizeof sfi, SHGFI_DISPLAYNAME)) return NULL;
	TCHAR partial[MAX_PATH] = _T("..\\");
	if (!_tcscat(partial, sfi.szDisplayName)) return NULL; 
	return PathCombine(corrected, path, partial);
}

Comment 55

[(no longer active)]

(In reply to comment #54)
> LPTSTR CorrectFolderCase(LPCTSTR path, LPTSTR corrected) {
>     SHFILEINFO sfi;
>     if (!SHGetFileInfo(path, 0, &sfi, sizeof sfi, SHGFI_DISPLAYNAME)) return
> NULL;
>     TCHAR partial[MAX_PATH] = _T("..\\");
>     if (!_tcscat(partial, sfi.szDisplayName)) return NULL; 
>     return PathCombine(corrected, path, partial);
> }

It would be freat if you can use this code in updater.cpp, and test to make sue that this fixes the bug, and then submit a patch!

Comment 56

[al_9x]

I am not set up for Firefox dev and don't have time for it now.  Why don't you make some sort of a try build?  Presumably you can just give me updater.exe?

Comment 57

[al_9x]

(In reply to al_9x from comment #56)
> I am not set up for Firefox dev and don't have time for it now.  Why don't
> you make some sort of a try build?  Presumably you can just give me
> updater.exe?

Are you going to do this?

Comment 58

[(no longer active)]

(In reply to comment #57)
> (In reply to al_9x from comment #56)
> > I am not set up for Firefox dev and don't have time for it now.  Why don't
> > you make some sort of a try build?  Presumably you can just give me
> > updater.exe?
> 
> Are you going to do this?

A try build from what?  Can you attach a patch please?

Comment 59

[al_9x]

(In reply to Ehsan Akhgari [:ehsan] from comment #58)
> (In reply to comment #57)
> > (In reply to al_9x from comment #56)
> > > I am not set up for Firefox dev and don't have time for it now.  Why don't
> > > you make some sort of a try build?  Presumably you can just give me
> > > updater.exe?
> > 
> > Are you going to do this?
> 
> A try build from what?  Can you attach a patch please?

I told you I am not set up for firefox dev and can't build it and don't have time to this now.

There is no point making patches if I can't check them.  What problem do you have with reviewing and incorporating the function I posted and making a try build?

Comment 60

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

By chance, have you tried to reproduce on a regular desktop? I have seen strange behavior with a few Windows API call on VM's previously that aren't reproducible when not running in a VM. This might explain why Ehsan doesn't see it when running Firefox and you do. I'd like this info so we can better prioritize this work in relation to other work.

In answer to "What problem do you have...", Ehsan is busy with other work that affects more users than this bug, he can't reproduce this bug on his system, and he doesn't have the time to investigate at this moment (he might in the near future and if he doesn't someone else will).

Comment 61

[al_9x]

(In reply to Robert Strong [:rstrong] (do not email) from comment #60)
> By chance, have you tried to reproduce on a regular desktop? 

Yes this was first noticed on a physical machine.

> I have seen
> strange behavior with a few Windows API call on VM's previously that aren't
> reproducible when not running in a VM. This might explain why Ehsan doesn't
> see it when running Firefox and you do. I'd like this info so we can better
> prioritize this work in relation to other work.

> 
> In answer to "What problem do you have...", Ehsan is busy with other work
> that affects more users than this bug, he can't reproduce this bug on his
> system, and he doesn't have the time to investigate at this moment (he might
> in the near future and if he doesn't someone else will).

Please reread comment 53, carefully.  The is no variance with respect to the api call (GetLongPathName). The variance is in the input to the api call.  The input already has the right case for him.

You reviewed this patch.  The intent of it was to correct the case of the install folder if it happens to be wrong, using the GetLongPathName api.

GetLongPathName api is known not to work on xp with folder lengths > 8.  Now that you know this, does it make sense to accept it as is, or perhaps it should be fixed to actually work as intended, on xp as well (comment 54)?  You have the code, is it really so much trouble to incorporate it and make a build?

Comment 62

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

(In reply to al_9x from comment #61)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #60)
> > By chance, have you tried to reproduce on a regular desktop? 
> 
> Yes this was first noticed on a physical machine.
Thanks

> 
> > I have seen
> > strange behavior with a few Windows API call on VM's previously that aren't
> > reproducible when not running in a VM. This might explain why Ehsan doesn't
> > see it when running Firefox and you do. I'd like this info so we can better
> > prioritize this work in relation to other work.
> 
> > 
> > In answer to "What problem do you have...", Ehsan is busy with other work
> > that affects more users than this bug, he can't reproduce this bug on his
> > system, and he doesn't have the time to investigate at this moment (he might
> > in the near future and if he doesn't someone else will).
> 
> Please reread comment 53, carefully.  The is no variance with respect to the
> api call (GetLongPathName). The variance is in the input to the api call. 
> The input already has the right case for him.
> 
> You reviewed this patch.  The intent of it was to correct the case of the
> install folder if it happens to be wrong, using the GetLongPathName api.
> 
> GetLongPathName api is known not to work on xp with folder lengths > 8.  Now
> that you know this, does it make sense to accept it as is, or perhaps it
> should be fixed to actually work as intended, on xp as well (comment 54)? 
> You have the code, is it really so much trouble to incorporate it and make a
> build?
Please reread comment #60, carefully. It should be fixed, there are bugs that affect more users that are currently a higher priority, and this will be fixed at some point in the future. Yes, at this time it is too much trouble to incorporate the snippet of code and make a build.

Comment 63

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

btw: future will likely mean sometime within the month

Comment 64

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

Closing old bugs as incomplete. If this is still an issue please file a new bug.