installer's faulty path syntax and destination directory [\\, windows registry]

VERIFIED FIXED in mozilla1.0.1

Status

SeaMonkey
Installer
VERIFIED FIXED
17 years ago
13 years ago

People

(Reporter: Pete Boyd, Assigned: Sean Su)

Tracking

Trunk
mozilla1.0.1
x86
Windows 98

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

17 years ago
I setup Win98 systems with different partitions for different jobs:
C: SYSTEM
D: PROGRAMS
E: DATA
F: TEMP
etcetera

I use TweakUI to set the 'program files' directory to D:\, this is a GUI way of configuring the registry thus:
HKLM/Software/Microsoft/Windows/CurrentVersion/ProgramFilesDir = "D:\"

[but confusingly at the same location there's also a
'ProgramFilesPath' = "C:\Program Files" which I don't know the meaning of]

when installing Mozilla (many builds in recent ?weeks/months?) and Mozilla offers a 'Destination Directory' I choose 'Browse...' to change the directory and Mozilla comes up with something like 'D:\\Program Files'. now, I forget exactly what Mozilla says here, it could actually be 'C:\Program Files' (I'll check soon), but the '\\' should instead be '\', and also the correct location of 'D:\' should be used from Windows' registry

Comment 1

17 years ago
Grace: do you have a Win98 machine to reproduce?

Comment 2

17 years ago
sorry, my 98 just died. 
I hope to have another one soon, currently only NT/2000/ME/XP beta and 95 at 
home

Comment 3

17 years ago
status?

Reporter do you still see this in the latest builds? Could you mention which
build did you see this in?

Comment 4

17 years ago
No data since opened in July. Also, WFM on Win98.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WORKSFORME

Comment 5

17 years ago
thanks Grey,
verified
Status: RESOLVED → VERIFIED
QA Contact: bugzilla → gbush
(Reporter)

Comment 6

17 years ago
sorry for the delay, I was waiting till I did a fresh install on a new machine.
using 0.9.5 I don't see this on my own machine, which is setup the same way I
setup new machines, however on a new system installing 0.9.5 I do still see this bug

Mozilla is correctly picking up my 'program files' setting of D: from, presumably:
HKLM/Software/Microsoft/Windows/CurrentVersion/ProgramFilesDir

but the full Destination Directory is still wrong:
D:\\mozilla.org\Mozilla

predictably, if I dont manually change this location and press 'Next' and answer
'Yes' to create the folder I get the error message 'Could not create folder'
(and this error window doesn't have a label, which I presume it should)

reopening the bug
Status: VERIFIED → UNCONFIRMED
Resolution: WORKSFORME → ---
(Reporter)

Comment 7

17 years ago
ZoneAlarm 2.6.231 does the same D:\\ business

as before, I see this on the new machine setup and not on my machine

the difference being that the new machine uses the 'D:' partition I specifically create for 'Program Files', and my machine uses the same partition as Windows: 'C:\programs'

both were set this way using Microsoft's TweakUI version 1.33

choosing 'c:\programs' from the TweakUI list sets a registry value of 'c:\programs'. choosing 'd:\' sets the value as 'D:\' and the installer picks up the value and adds its '\' at the end

you could say this is the fault of TweakUI being inconsistant. I would have thought Mozilla should have a workaround for this as TweakUI is a pretty common utility for Windows people to be using, its unsupported by MS but comes on some original Windows install discs and is available from their web site; it gathers together a load of essential user interface settings that would otherwise need to be set by hand editing the registry or using someone else's comparable utility

setting a seperate drive for Program Files is also not an uncommon thing to do in Windows, whether its on a seperate partition on the same drive or on a completely seperate drive, for performance gain thru lack of fragmentation and concurrent disk access
(Reporter)

Comment 8

17 years ago
Windows 2000 doesn't see this bug as (the same version of) TweakUI in Windows
2000 won't allow you to choose a drive letter on its own to install programs to,
needing instead a directory to point to aswell

Comment 9

17 years ago
Have Win98 machine now and will try to reproduce
Assignee: ssu → syd
(Reporter)

Comment 10

17 years ago
by my reckoning (and with some help from Timeless), I reckon...

the code for this bug lies in:
http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/windows/setup/extra.c

and these lines from it:
6536   else
6537     return(FALSE);
6538
6539   return(TRUE);

want to change to either this:
6536   else
6537     return(FALSE);
6538
6539   /* remove backslash (i.e. if "PROGRAMFILESDIR" is 'D:\',
6540    * the installer will try and fail with 'D:\\')
6541    */
6542   if(szVariable[strlen(szVariable) - 1] == '\\')
6543      szVariable[strlen(szVariable) - 1] = '\0';
6544
6545   return(TRUE);
6546   }

or this:

6536   else
6537     return(FALSE);
6538
6538   /* remove backslash (i.e. if "PROGRAMFILESDIR" is 'D:\',
6539    * the installer will try and fail with 'D:\\')
6540    */
6541   RemoveBackSlash(szVariable);
6542
6543   return(TRUE);
6544   }

I'll now look into how to compile and test this myself and create CVS diffs. this is all new to me so bear with me
On instructions from timeless, confirming bug to get it on radar.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

17 years ago
Target Milestone: --- → M1

Updated

17 years ago
Target Milestone: M1 → Future
(Reporter)

Comment 12

16 years ago
I was enthusiastic that I could solve, compile, test and submit a patch for this
bug myself, but didn't realise I had to use Microsoft Visual C++ to do so, thus
paying plenty of money to Microsoft, more than I could afford or would want to
pay. I've been following the reconfiguration of the build process to allow a
free compiler to be used, with relief, but from what I see that ability isn't
ready yet. so, I have to pass-up on finishing off this bug myself. hopefully
someone else will have the time to get it in before 0.9.8, its a simple fix,
details of which I've already submitted. 
sorry, but I'm looking forward to the build process being set Free
Found this via new bug 134167, thanks for fingering the duplicate. Reassigning
to Curt, CC'ing ssu for comment, nominating because this will be a small but
non-zero source of install failure.

Looks like DecryptVariable() in extra.c only deletes a trailing slash from
WIZTEMP and TEMP, everything else it trusts to be correct in the windows
registry. DecryptVariable() should ensure that all ...PATH variables are in the
same format and don't have a trailing slash. 

Since not all the variables are paths you can't, unfortunately, just stick
common slash-removing code at the end of the routine. :-(

tangent: Several of the supported variables are not documented in config.it
Assignee: syd → curt
Keywords: nsbeta1
*** Bug 134167 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt3]
Target Milestone: Future → mozilla1.0.1

Comment 15

16 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-

Comment 16

16 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
(Reporter)

Comment 17

16 years ago
adding to Summary to aid searchers who'll find this in their Mozilla 1.0.0 and
Netscape 7.0
Summary: installer's faulty path syntax and destination directory → installer's faulty path syntax and destination directory [\\, windows registry]
*** Bug 148061 has been marked as a duplicate of this bug. ***

Comment 19

15 years ago
Created attachment 113518 [details] [diff] [review]
eliminate duplicate backslashes when substituting PROGRAMFILESDIR

The patch introduces a DecryptPathString extending the DecryptString with
elimination of duplicate backslashes.

It is no really satisfying because it leaves other path variables (which are
potentially affected, too), untouched - it only handles the one special case
where the config.ini containing the default install path
([PROGRAMFILESDIR]\mozilla.org\Mozilla) is affected.

I will seek for advice from the owners of the patched file(s) to see if this
would be an acceptable solution for the bug here.

Comment 20

15 years ago
Comment on attachment 113518 [details] [diff] [review]
eliminate duplicate backslashes when substituting PROGRAMFILESDIR

curt, could you please have a look at the patch when you have a free minute? :)
Attachment #113518 - Flags: review?(curt)
(Assignee)

Comment 21

15 years ago
curt's no longer working on the installer. over to me.
Assignee: curt → ssu
Status: ASSIGNED → NEW
(Assignee)

Comment 22

15 years ago
Comment on attachment 113518 [details] [diff] [review]
eliminate duplicate backslashes when substituting PROGRAMFILESDIR

Frank, thanks for helping us out with a patch.

Unfortunately, the patch will break if the chars are multibyte chars.

You should rewrite it to use CharNext() and/or CharPrev() to get to the next or
previous characters. These are multibyte safe functions.

Also, can you change DecryptPathString() to be more descriptive? something like
RemoveDoubleBackslashesFromString()?

This function is better called from inside DecryptString(), inside the
'if(bDecrypted)' check (towards the end DecryptString() function) right after:
  lstrcpy(szOutputStr, szResultStr);
Attachment #113518 - Flags: review?(curt) → review-

Comment 23

15 years ago
Created attachment 113921 [details] [diff] [review]
updated patch

Sean, thanks for looking at the patch! I created a new version complying to
your three requests ....
Attachment #113518 - Attachment is obsolete: true

Updated

15 years ago
Attachment #113921 - Flags: review?(ssu)
(Assignee)

Comment 24

15 years ago
Comment on attachment 113921 [details] [diff] [review]
updated patch

sorry it took so long to get to this patch.

r=ssu
Attachment #113921 - Flags: review?(ssu) → review+

Comment 25

15 years ago
Comment on attachment 113921 [details] [diff] [review]
updated patch

Sean, thanks for looking at this :)

dveditz, do you mind super-reviewing this?
Attachment #113921 - Flags: superreview?(dveditz)
Comment on attachment 113921 [details] [diff] [review]
updated patch

>+  while (pSearch != pSearchEnd)

I'd be more comfortable with '<' to prevent run-away loops in case some future
change causes pSearch to move more than one at a time (as CharNext can, in
fact).

>+        // no increment of pSearch here - we want to continue with the same character,
>+        // again (just for the weird case of yet another backslash...)

So you're not removing DoubleBlackslash so much as collapsing
MultipleBackslash.
Attachment #113921 - Flags: superreview?(dveditz) → superreview+

Comment 27

15 years ago
Created attachment 118087 [details] [diff] [review]
modified loop condition

> >+  while (pSearch != pSearchEnd)
> I'd be more comfortable with '<' to prevent run-away loops in case some
future
> change causes pSearch to move more than one at a time

Agreed. Also considering a broken string where the "last byte" is a
(incomplete) part of a multibyte character, this seems to be a good idea, so I
updated the patch. Thanks!
Attachment #113921 - Attachment is obsolete: true
(Assignee)

Comment 28

15 years ago
patch checked in for frank.schoenheit@gmx.de.  closing bug as fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago15 years ago
Resolution: --- → FIXED

Comment 29

15 years ago
Reporter,  Can you verify?

Comment 30

15 years ago
verified code fix
Status: RESOLVED → VERIFIED
(Reporter)

Comment 31

15 years ago
my apologies. its been a long time since I administered any machine running
Windows 98 so couldn't verify this bug fix. everythings running Windows 2000 now
and in a directory that would probably work around this bug (D:\programs).
allthough having said that, I did upgrade software on an old machine still
running Windows 98 last week and something like 50% of the software I tried to
install had the same problem that this bug descibes
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.