Closed Bug 125106 Opened 19 years ago Closed 18 years ago

general nsIFile converter change hosed XPInstall intl filenames

Categories

(SeaMonkey :: Installer, defect, P1)

x86
Windows XP
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: ruixu, Assigned: ssu0262)

References

Details

(Keywords: intl, regression, Whiteboard: [adt1][not fixed on trunk])

Attachments

(14 files, 4 obsolete files)

3.16 KB, text/plain
Details
114.84 KB, image/jpeg
Details
19 years ago
114.84 KB, image/jpeg
Details
430 bytes, text/plain
Details
66.66 KB, text/plain
Details
165.31 KB, patch
dougt
: review+
chofmann
: approval+
Details | Diff | Splinter Review
150.77 KB, patch
dougt
: review+
Details | Diff | Splinter Review
11.40 KB, image/jpeg
Details
63.36 KB, image/jpeg
Details
46.82 KB, text/plain
Details
51.69 KB, image/jpeg
Details
12.75 KB, text/plain
Details
562 bytes, application/x-xpinstall
Details
195.89 KB, patch
dveditz
: review+
dveditz
: superreview+
sspitzer
: approval1.4b+
Details | Diff | Splinter Review
Build: 2002-02-12

Steps:
1. Install Mozilla on JA WinXP.
2. After installation, observe desktop and Start Menu.

Actual:
1. Mozilla icon is not on the desktop.
2. Mozilla program folder is an empty folder.
Keywords: intl, nsbeta1, regression
QA Contact: bugzilla → ruixu
need to see the install log please, might help us understand the problem.
Status: NEW → ASSIGNED
Attached file install log file
An install log file is attached, please let me know if need more help. Thanks.
using build 2002021503 en on XP, I see both a Mozilla icon ( and N6.2) on the
desktop. In the program folder I see Mail, Mozilla, Profile Manager along with
license and readme (which is another bug-it does not exist)  

Must be JA machine/build that comes into play here
I cannot repro this bug on EN XP as well.
It only can be reproduced on localized XP, e.g. JA XP, SC XP.
Syd,

I checked with Rui on this bug- watched an install- this is as normal user vs 
administrator or power user on XP.  The icon is NOT on the desktop, nor are the 
icons in start menu/netscape6.2/

I tested on English XP as normal user and despite warning- that I did not have 
admin privileges it did install and put icons where expected.
Rui believes it has to do with localization and not interpreting double bytes 
(?)
FYI, in some localized OS, The folder name "Start Menu" and "Desktop" are
localized, they are not in English.
Keywords: nsbeta1nsbeta1+
Target Milestone: --- → mozilla1.0
over to Dave
Assignee: dveditz → dprice
Status: ASSIGNED → NEW
Info in bugscape 12401 could be helpful. Looks like same issue here.
nsbeta1- per adt triage
Keywords: nsbeta1+nsbeta1-
Target Milestone: mozilla1.0 → ---
Just curious, is there any reason to triage this bug from "+" to "-"? 

This bug happens not only in mozilla builds but also in the trunk builds. And, 
it not only happens on localized WinXP, but also happens on other localized
Windows platfroms, e.g. NT, 2K, 98, Me, etc.. I wonder if nsbeta1 can be changed 
back to ''+" since runing our browser from desktop icon and start menu are very
common user operations, thus very important.
Summary: Cannot install Mozilla desktop icon and program folder properly → Cannot install desktop icon and program folder properly on localized Windows
Severity: normal → critical
Agree to plus it. The worst thing is, user can not even manually delete those 
garbage folders from their system (See the screenshot above #6.) They have to 
re-format the whole harddisk, or use some special tools like Norton Disk 
Utilities to remove them. 
This bug only affects the ability to create a shortcut on the desktop. Since at
least some users don't even want the desktop icon it can't be that big a deal.
Also, we've lived with this situation for all previous releases. On the flip
side, fixing this would require re-writing core XPInstall APIs which sounds 1)
risky  and 2) a lot of work at this late date for a bug we've lived with this long.

Still, if you know of a hidden programming resource who has time to fix this let
the ADT know. I can point people at the broken code.
rchen and myself (yxia) can have a try. Please let us know the details of the 
broken code, any information to fix this bug will be very helpful. Thanks.
Without this bug fixed, we can not ship any double-byte localized version.
Dan, do you mind posting the broken code? We will do as much as we can to fix 
it.
Also, this problem is NOT old issue which "a bug we've lived with this long", 
it's a new problem just from weeks ago. It means some recent checkins broke the 
code. Am I correct, Rui?
Dan, I verified that there was no such a problem in NS 6.21 release. The 
shortcuts were created properly in the Japanese folders "Desktop" and "Start 
Menu".   

This bug is a regression. We can look at what has been changed since 6.21. It 
will give us a better picture. 
Renominated for nsbeta1, since it's a serious regression from the previous release.
Keywords: nsbeta1-nsbeta1
> Also, this problem is NOT old issue which "a bug we've lived with this long",
> it's a new problem just from weeks ago. It means some recent checkins broke 
> the code. Am I correct, Rui?

Yes, Ying and Ray, you are right. This is a regression. It is not 
reproducible even using 20020130 trunk build.
Keywords: nsbeta1nsbeta1-
Hmm, the checkin is between 20020130 and 20020212. It shouldn't be hard to find 
out what causes it.
Keywords: nsbeta1-nsbeta1
changing to nsbeta1+
Keywords: nsbeta1nsbeta1+
Can you narrow this down a bit?  a 2 week period is kind of big.  Does it happen
before of after 2/6/2002?  An exact date would be even better.

Thanks
It also can be reproducible with 20020202 trunk build, the checkin is between 
20020130 and 20020202. Thanks.
In browser.jst, it looks like getFolder messes up the double byte string. In 
line 150, the string in fTemp valuable is still correct but not in fFolderpath. 
We may skip getFolder call and pass the value directly to fFolderPath. 

      fTemp       = szStartMenuPrograms + "\\" + scFolderName;
      fFolderPath = getFolder("file:///", fTemp);
According to aa comment in the duplicated bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=125958#c5

It seems 2002-02-01-06-trunk or later builds cause these errors, 
2002-01-31-11-trunk or earlier don't have this bug.
The most likely culprit in
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=01%2F31%2F2002+11%3A00%3A00&maxdate=02%2F01%2F2002+08%3A00%3A00&cvsroot=%2Fcvsroot

is Alec's checkin for bug 100676, removing the uconv dependency from xpcom.

Prior to that the install scripts "worked" because the native stubs didn't
include uconv, and thus we got a "broken" conversion from
ascii->fake-ucs2->ascii. Apparently somewhere in there we now have the
filesystem getting converted to a real ucs2, but somewhere else still treating
it as null-padded 8-bit. This could be hard to track down.

This would also imply that XPInstall updates would have been broken in the old
code since uconv certainly did exist and do the right thing while the browser
itself is running. This could be proved by testing with the update.html pushed
to each sweetlou build directory on a system that also exhibits this bug. It
also means the new update notification feature won't really work on those
systems until this is fixed.
adding to whiteboard adt1
cc: richie close
Whiteboard: adt1
It seems Dan is right. 

I digged into the code and found out:
In InstallGetFolder function (in the file nsJSInstall), the string passed from 
"fTemp" which contains double byte folder name is not processed in a correct 
Unicode or native code. For exmple, the double character "su" (0x8358 in 
shift-JIS, 0x30B9 in unicode) becomes four bytes 0x83005800 in nsString b1  
after calling ConvertJSValToStr.
  if(argc >= 2)
  {
    ConvertJSValToStr(b1, cx, argv[1]); 

After that, there are converting between nsString and nsCstring. It could be 
messed up somewhere.

To me, the solution could be:
1. Convert the string into Unicode correctly and process through the 
application. But according to Dan, somewhere is still using fake-ucs2 
conversion? It may be risky to break other code? 
 
2. Since the need here is just the folder name reading and writing, can we skip 
the conveting and store the data in cstring? Installer reads the folder name and 
write it to the registry in the same machine. It seems not necessary to have the 
string convertsion. This is my understanding amd I could be wrong.
More info:

NSinstall uses JS_ValueToString to read the JS value. All the character becomes 
wide char blindly as 0x8358 in shift-JIS becomes four bytes 0x83005800. 

However before NSinstall uses STRING_TO_JSVAL to write the path to JS value, it 
converts the path to Unicode by calling GetUnicodePath which returns the genenue 
unicode. But STRING_TO_JSVAL just reverses what JS_ValueToString does. It 
expects the fake-ucs2 like 0x83005800. The problem happens right here!

Dan's comment in ToString already points out the potential problem back to May 
2000.

nsInstallFolder::ToString(nsAutoString* outString)
{
  //XXX: May need to fix. Native charset paths will be converted into Unicode 
when the get to JS
  //     This will appear to work on Latin-1 charsets but won't work on Mac or 
other charsets.
  //     On the other hand doing it right requires intl charset converters
  //     which we don't yet have in the initial install case.

  if (!mFileSpec || !outString)
      return NS_ERROR_NULL_POINTER;

  nsXPIDLString  tempUC;
  nsresult rv = mFileSpec->GetUnicodePath(getter_Copies(tempUC));  
The quick fix can be:

1. Instead of calling STRING_TO_JSVAL, call some other function which can handle 
unicode properly. 
Or 
2. Do not convert the path to unicode and pass the fake-ucs2 to STRING_TO_JSVAL.

Any comment?
The toString code is mostly a red-herring, it's only used for debugging output
and the install log. This is why uninstall fails, but it's probably not the
cause of this problem.

The problem here is that we used to get native chars out of the registry
(null-padded to double-byte) and then create a filepath out of them (stripping
the null padding). The strings weren't meaningful in the script itself, but it
worked out.

But bug 100676 fixed the file code apparently, so now we'll have to fix the
registry code and anywhere else that relied on a round-trip for native chars
working OK.

We could also add extra getFolder() targets, like "Win Desktop" and "Win Program
Files", etc -- the directory service seems to provide these so it wouldn't take
much work and then the paths would be correct. Probably we should do both
adding pgorman
Whiteboard: adt1 → [adt1]
*** Bug 125958 has been marked as a duplicate of this bug. ***
Dan - Ray is on vacation, but mentioned that you're working on a patch for this.
Can you please provide status? When do you expect to make the patch available?
thanks!
Is there any movement on this bug? It's really important we get this one fixed.
Or we will NOT be shipping Japanese or Chinese Mach V Betas. 
Please provide status. thanks.
cc: msanz
I'm working on this. It's slow going because we had a lot of code taking
advantage of the previously broken conversions in xpcom/io.

Part of the fix is to define new getFolder() targets for the shortcut and
desktop directories so the actual paths are opaque to (and therefore undamaged
by) the script. Since these won't exist in older versions of netscape we will
have to decide whether we're OK not be able to upgrade people using XPInstall.
Since we don't have a "SmartUpdate" site and enterprise versions don't yet
support "Remote AutoUpdate" I think that's OK.

If it's not we can complicate the scripts by writing the code twice, using
getFolder() when it works, and keeping the old code in order to upgrade folks
using 6.x
Assignee: dprice → dveditz
Status: ASSIGNED → NEW
FYI: UTF convertions could spoil JA chars for it has 2 types on JA KAKATAKANA
chars. Win directory names on WinNT4/95/98/ME is different from names on
Win2K/XP.
Is this also reproducable on Chinese Windows?
Yes, it is also reproducable on Simplified Chinese, Traditional Chinese and 
Korean Windows, (both "Startmenu" and "Desktop" are translated with DBCS 
character).

Also tried on a Western Windows, it is not reproducable on German Win98SE even 
"Startmenu" is translated and it contains high-ASCII character.
It'd be more likely to be a problem on a non-Latin-1 single-byte language.
Latin-1 is the first plane in Unicode so the bogus conversions turn out to do
the right thing.
roy yokoyama, please help dveditz
let's see what I can do.
As mentioned in comment 36, I'm working on this. Please don't launch in blindly
as I've got half a fix going already. I was held up last week by tree closures
and more important blocking bugs.
Let me know if you need my help, dveditz.
It is nice to know the progress is on going. Intl. team is eager to see it 
fixed. 
Dan, can you let us know approximately when the patch will be ready? thanks. 
adding metabug blockage
Blocks: 136384
Taking this on my hitlist.  As rchen pointed out below, we need an ETA for the
langing of a fix.  Dan?
Whiteboard: [adt1] → [adt1] ETA UNKNOWN
Whiteboard: [adt1] ETA UNKNOWN → [adt1] ETA UNKNOWN[hitlist]
>roy yokoyama, please help dveditz
dveditz@netscape.com: please don't mis read my comment. Help could mean from
provide test cases, code review, debugging, verify fix, brainstorm solution. We
are a team together. Helping each other is very normal. 
>Please don't launch in blindly as I've got half a fix going already. 
That is not what I ask yokoyama to do, otherwise, I will talk to him in private
instead put a comment in public. We just want to let me know that if you need
some developers to help you for this issue (in different form), he is here to
support you. (not this week, he is on vacation this week)
I can test at least(I have WinMe JA box here.). I'm quite new to here, so I'm
not sure how I can support you. On the other hand, am anxious to fix this. So,
Please note whatever you have doubts. (my eyes are debugging on spare time for
this...)
ADT talked to Dan today.  He said he can land a fix by 4/12, then needs to go
thru  review process.
Whiteboard: [adt1] ETA UNKNOWN[hitlist] → [adt1] ETA 4/19[hitlist]
04/12 was last friday. did you mean 04/17?
Whiteboard: [adt1] ETA 4/19[hitlist] → [adt1] ETA 4/19[hitlist] [m5+]
Still repro on 2001041903(trunk) / WindowsMe JA .

I care a little bit about nsLocalFileWin.cpp.
attachment 67303 [details] [diff] [review] (sr on 01/31)
Isn't it possible that it still requires uconv in some l18n case?

I think this bug depends on bug 100676.
Dan, as this is a critical bug to beta, and the ETA of 4/19 has passed, can you
say a new ETA to land this fix?
Blocks: 126276
Whiteboard: [adt1] ETA 4/19[hitlist] [m5+] → [adt1] ETA 4/26[hitlist] [m5+]
How is the patch going on this?  Any new ETA since 4/26 has passed?
Attached file install_wizard1.log
WFM now. 2002042708(trunk) / WindowsMe JA
I did install/deinstall twice and both created/removed desktop icon and
startmenu folder.
But refering line 545 to 549 in install wizerd log, seems installer is trying
to create something improper.
TNX.
Attachment #81316 - Attachment mime type: application/octet-stream → text/plain
Changing eta to today (4/29) based on conversation with dveditz.
Whiteboard: [adt1] ETA 4/26[hitlist] [m5+] → [adt1] ETA 4/29[hitlist] [m5+]
*** Bug 141099 has been marked as a duplicate of this bug. ***
*** Bug 140712 has been marked as a duplicate of this bug. ***
adding 4/30 to the eta based on today's conversation with Dan.
Whiteboard: [adt1] ETA 4/29[hitlist] [m5+] → [adt1] ETA 4/30[hitlist] [m5+]
This -w patch fixes the broken Wide/narrow filename conversions going on in
XPInstall exposed by the fix to bug 100676. I need some reviews, and I
especially need the i18n/l10n folks to try this on non-ISO-8859-1 windows OS's.


The patch's size is daunting, but if you look it's mostly the same pattern over
and over. We have extensive test cases for the XPInstall API which should help
quell management fears. Start at
http://www.mozilla.org/quality/smartupdate/xpinstall-testinfo.html with the
actual test cases found at
http://www.mozilla.org/quality/smartupdate/xpinstall-trigger.html
Since the test cases linked above are almost all ASCII you're not testing the
fix (other than regression testing) unless your Mozilla build is sitting in a
directory that has non-Latin-1 characters in the name (or for some tests, in
your user profile directory path).

But don't let the lack of a localized OS stop anyone -- regression testing is
good at this stage of the game.
Status: NEW → ASSIGNED
Whiteboard: [adt1] ETA 4/30[hitlist] [m5+] → [adt1] ETA 4/30[hitlist] [m5+] need reviews
One more thing, the patch is a MOZILLA_1_0_0_BRANCH patch, it will most
definitely NOT apply to the trunk where the nsIFile interface has completely
changed.
Whiteboard: [adt1] ETA 4/30[hitlist] [m5+] need reviews → [adt1] [hitlist] [m5+] [ETA 4/30] need reviews
dan: if we just build mozilla and run it on CJK os we won't exercise this patch,
right. We need to build a installer from the patch and run the installer than we
can test the patch, right? Could you tell us how to do that ?
I assume we need to do the following
1. get a branch tree
2. apply your patch.
3 and then ???? please fill in this part. how to build rebuild patch. Do we need
to rebuild the whole mozilla ? or just some sub directory
4. run the installer over CJK os and test

Please tell us what we should do in step 3. 

yokoyama, once dan fill the info about step 3, please try it . Give your build
result to IQA to run on different windows platform too. 
Comment on attachment 81829 [details] [diff] [review]
Patch to fix charset/filepath bogosities

the patch looks great to me - I see lots of great cleanups of lossy code, and
overall I think it all looks quite consistent.

(I was thrown off by the *Unicode* methods, but then I figured out it was
MOZILLA_1_0_BRANCH, like dan later said)

my only issue is that I personally think that UCS2ToFS and FSToUCS2 should have
nsAString/nsACString-based "out" parameters to avoid the excess copying.. that
would allow you to use nsXPIDL[C]String less, and use ns[C]String or some other
slightly more useful string class. I'm guessing you just copied and pasted from
the nsLocalFile* implementations..

but sr=alecf with or without the change to nsA[C]String.
Nice cleanup.
Attachment #81829 - Flags: superreview+
Syd or dveditz, could you create a test build so IQA can try it before checking
in to the branch?
After you patch the code you'll need to rebuild mozilla to get the changes into
the xpinstall engine (unless you know a shortcut for building that).  

You may want to go to mozilla\xpinstall\wizard\windows\nsztool and do "nmake /f
makefile.win" to build this tool which is required by the installer build
script. I don't know why, but it doesn't always get built for me.

Then, to build the installer go to mozilla\xpinstall\wizard\windows\builder and
run "perl build.pl".  This will build the stub, scripts, etc. and drop the built
installer into mozilla\dist\WIN32_D.OBJ\setup.
I try this without dan's patch. But I got error when I try "perl build.pl". IS
this only working on opt tree? well this work with debug tree. Could someone
know how to do this build a full installation dir with dan's patch  for IQA to
test ?
Comment on attachment 81829 [details] [diff] [review]
Patch to fix charset/filepath bogosities

what nhotta said.  Lets make sure this is tested prior to landing on the
branch.

What about the trunk.  Dan, how much still must change after the nsIFile/utf8
changes?  Can you push a patch which addresses the trunk?

minor nit:
In nsInstallFileOpItem::~nsInstallFileOpItem(), lets get rid of the commented
out deleting.  If we need to "remember" this we always have cvs.  If you
disagree, lets *at least* comment what is going on in this destructor.

assuming that we get iqa testing prior to checkin, r=dougt
Attachment #81829 - Flags: review+
If people who aren't used to building this are having problems building this,
could Dan or someone on the install team make a test build for iqa with this
patch in it?  It sounds like we might not have the opportunity to land this on
the trunk first, but if we could get a branch test build, we could use that.

To build the patch go to the top of the xpinstall subtree and do "nmake /f
makefile.win"

To build the installer you must have ActiveState perl, the perl that some people
get with cygwin is apparently incomplete or differently configured from what the
build scripts require.

The installer will end up in $(DIST)\install. To test just run setup.exe in that
directory. To distribute grab the blob out of the "sea" subdirectory.

optimized or debug doesn't matter.

A trunk patch will be forthcoming, probably tomorrow or maybe late tonight
dan- can you just build it for us?
we are running out of time, not a good time to debug why my machine have perl
incompatable with the script. 
Dan - please just build it and provide use test bits. Thanks
Test build available to Netscape folks at
  ftp://foo@slip.mcom.com/../../u/dveditz/125106.exe

replace "foo" with your username, and you'll be prompted for your LDAP password.
The file's too big for the warp webserver so http://warp/u/dveditz/125106.exe
won't work.
Also should note that you don't need an installer to test this build. XPInstall
itself was broken, so you can test the patch in development builds by trying to
install the .XPI files from the ftp server. Pick the ones that try to install
shortcuts, like browser.xpi and mail.xpi

Doing this will tend to mess up your development build and it probably won't run
afterwards unless you are careful to update all of the .xpi's, but at least you
could find out that the shortcuts etc. are created OK. It's easy enough to zap
dist and re-copy the built libraries out of your tree.

And as linked to before, the XPInstall tests would be a good thing to run on a
localized OS with Mozilla running from a path with non-Latin-1 chars in it. The
originally described "shortcut" problem is just the tip of the iceberg of what
got broken.
dan: I tried "ftp://yokoyama@slip.mcom.com/../../u/dveditz/125106.exe";
     but got "page doesnt exist" err in Chinese. No password prompt.
     What am I missing?
Just found I can't see slip over SERA either, only from inside work. Any ftp
server that can see the work accounts should work, but I don't know which ones
are available. Anyone at work with NFS can mount the /u users drive and get the
file directly.

Made the comment changes dougt requested.  Seeking approvals.
Keywords: adt1.0.0, mozilla1.0
Whiteboard: [adt1] [hitlist] [m5+] [ETA 4/30] need reviews → [adt1] [hitlist] [m5+] [ETA 4/30] need branch approval
Humm, the link is paste in #75 works now.  
Humm, the pasted link in #75 works now, but desktop icon and program folder
don't get created on WinXP-SC using 
"ftp://yokoyama@slip.mcom.com/../../u/dveditz/125106.exe";

i thought i should reproduce the orginal bug by using 
the trunk build (05-02).  
Now trunk build creates the icons and program folder!!

ruixu: can you try the trunk build with XP-Ja?
branch patch merged with the trunk nsIFile changes from bug 129279
Man there were a lot of leaks! The code was originally written using nsFileSpec
which was written to manage all the memory buffers it handed out through GetPath
and GetLeafName, etc. Apparently during the nsFileSpec->nsIFile conversion it
was done somewhat automatically since there was tons of it in the file-heavy
install code, and a lot of places where the method names were the same (so
compiler errors didn't flag it as needing work) the change in semantics of who
owned what weren't dealt with.

There are still a couple leaks in the branch patch that I just noticed, but it's
a lot better and I don't want to start reviews over with another patch on the
branch. We've been living with these for years, installation is rare and it
won't kill us to leave them. The trunk gets it right (Thanks Darin!)
I tested build 125106.exe, and got an error message as attached right after
installation and before launching browser. Spot-checked on the following
platforms:
1. on JA 98SE, NT4 and 2K, no desktop icon and program folder can be installed.

2. on SC 98SE and Me, desktop icon and program folder are installed properly.
I also tested trunk build (20020502), it is getting better, but still has some
problems. 
I spot-checked JA 98SE, NT4, 2K and SC 98SE, Me, XP, the desktop icon, the
icons in start menu and the icons in quick start of taskbar can be installed.
In program folder, some icons can be installed, some icons are lost, and the
corrupted folders are still reproducible on JA NT4 and 2K, please refer to the
attachment, but not reproducible on SC XP.
Comment on attachment 82068 [details] [diff] [review]
patch merged to the trunk

dougt
Attachment #82068 - Flags: review+
The trunk has not received this patch, but it did get the fixes from bug 129279
which undoes some of the breakage. But anything that was windows-only
(shortcuts, directory names we get from the windows registry) is still broken.

Please attach the most recent install_wizard#.log from the 125106.exe install.
More updates:

Tested build 125106.exe on SC NT4, SC 2K and JA XP, no desktop icon and program
folder can be installed. Only see the Mozilla icon in quick start area of
taskbar is installed on SC 2K and JA XP.
Can't tell a whole lot from the install log output, because the install log has
independent Unicode->ASCII conversion issues that I skipped for this bug. the
string of dots disturbs me, though -- not what I'd expect from the
NS_LossyConvertUCS2toASCII that's done in the logging routine.

I'm going to need help from someone who can debug this on a JA machine.
ask shanjian to help on this as the highest priority.
Shanjian has a SC Win2K, which Rui says appears fixed by the patch. Need someone
who can debug this on Japanese windows, the one that appears to still have a
problem. Or we can call this a partial win and check in, leaving the additional
Japanese subset of the problem for an additional fix.
1. Should we use trunk to debug if the patch has been checked in (accroding 
comment#79)? 
2. Can you point out the specific area that we should look at? "getFolder"? 

This screen shot is to clarify where we should take care to fix and test for
this bug.

This screen shot is created on a English Win2K with test build 125106.exe. It
might be changed on some different Windows platforms.

In some Windows platforms, some area may not be displayed, e.g. taskbar icon on
Win9x and WinNT4.
>> Shanjian has a SC Win2K, which Rui says appears fixed by the patch. 

No, it is still reproducible on SC Win2K. Please see Comment #85 for the 
detailed test results.
the build I uploaded was a Mozilla build, so it may not spam itself into as many
places as a Netscape build.

Shanjian has found at least one error in nsFileURL, an obsolete xpcom/io class
we were depending on -- it uses "lossy" conversions. I'm looking into it.
Removing adt1.0.0, until we have a new patch with reviews.
Keywords: adt1.0.0
Whiteboard: [adt1] [hitlist] [m5+] [ETA 4/30] need branch approval → [adt1] [hitlist] [m5+] [ETA 05/03]
There were several possible ways to fix the fact that nsFileURL using
NS_LossyConvert internally.

Fix nsFileURL: That lives in xpcom and other people use it -- risky thing to
re-write.

Replace use of nsFileURL with local fixed code: would take a lot of work
duplicating code -- risky

Don't use getFolder("file:///") in scripts: would really like to, and started
down that path, but a lot of code needed changing in the scripts, and there was
one spot I couldn't get around using a file URL -- risky!

Hack nsInstallFolder to do the correct thing on windows: small patch. Leaves
broken nsFileURL on Mac and Unix, but our current scripts on those platforms
don't use the feature. The fix is #ifdef XP_WIN, everything else is status quo.
This attachment is NOT a patch -- do not attempt to apply it. This is the diff
between attachment 81829 [details] [diff] [review] (branch patch) and my current branch patch with the
nsFileURL usage tweak.

The changes are
 - fixed a leak
 - removed commented out dead code requested by Doug
 - changed nsFileURL usage, fixing XP_WIN and making the existing brokenness
   explicit on the other platforms.
I know both ADT and drivers want this fixed, I know this doesn't break English,
and it definitely improves Asian windows if not entirely fixing it. I haven't
heard officially from drivers but I believe checking this in is the right thing
and we're out of time.
Keywords: fixed1.0.0
adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 brnach. thanks!
Keywords: adt1.0.0+
Whiteboard: [adt1] [hitlist] [m5+] [ETA 05/03] → [adt1] [hitlist] [m5+] [ETA 05/03] [Needs r/sr/a=]
ruixu/shanjian - can we get this verified on the branch asap today. thanks!
It is not reproducible with branch build 20020503 on SC 95 OSR2, JA 2K, JA 
98SE, SC XP and SC 98SE so far, will test the rest platforms.

BTW, bug 141858 (Unexpected '\Setup\setup.exe' dialog after installing browser) 
can Reproduced on each platform above with branch 20020503.
blocker bug http://bugscape.netscape.com/show_bug.cgi?id=14744 may be related to
these changes.
darin- is 14744  linux only changes?
More updates:
Tested branch build 20020503 on the rest platforms: 
JA 95 OSR2, JA Me, SC Me, JA NT4, SC NT4, SC 2K, JA XP, and also spot-checked 
it on TC 98SE (non-coverage OS), this bug is not reporducibe.

Bug 141858 (Unexpected '\Setup\setup.exe' dialog after installing browser) 
can also be reproduced on each platform above with branch build 20020503.

Bug 125104 (JA WinXP: Cannot uninstall Mozilla completely) still can be 
reproduced on JA XP with branch build 20020503.

Bug 137656 (Uninstall failed if installation folder name contains double byte 
characters) is not reproducible with branch build 20020503 on above localized 
Windows platforms.

Will update each bug report, and thanks for great fix!
ssu/samir - if we end up backing this one out to resolve the linux installer
issue (bug 142107), pls remove the fixed1.0.0 keyword.
Leaf backed this out of the branch yesterday.  Removing fixed1.0.0 keyword.
Keywords: fixed1.0.0
Whiteboard: [adt1] [hitlist] [m5+] [ETA 05/03] [Needs r/sr/a=] → [adt1] [hitlist] [m5+] [ETA 05/06] [Needs New Patch]
Leaf backed this out of the branch on Friday becaused it caused install failure
on Linux.  Removing adt1.0.0+ keyword.
Keywords: adt1.0.0+
i thought he backed it out because it didn't have approval
Keywords: adt1.0.0
We need reviews and super-review here and we'll reconsider for RC2 (now that bug
142107 is fixed) 
asa we will just reland what dan landed.  we already have r/sr/a for that, don't we?
adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 brnach, pending
Doug's local testing and river's approval. Pls check this in asap (reviews from
dougt and alecf for Patch to fix charset/filepath bogosities still apply), then
add the fixed1.0.0 keyword.
Keywords: adt1.0.0adt1.0.0+
Priority: -- → P1
Whiteboard: [adt1] [hitlist] [m5+] [ETA 05/06] [Needs New Patch] → [adt1] [hitlist] [m5+] [ETA 05/06]
Target Milestone: --- → mozilla1.0
a=chofmann for the 1.0 branch. let's get as many eyes on the patch and the
builds that will spin this afternoon as we can.
Attachment #81829 - Flags: approval+
this patch looks good to me.
nsInstallFolder.cpp is the change to cope with localized CJK system installation. 
It looks fine to me from i18n standpoint. 
in total agreement


Relanding Dan along with the Pink mac fix and the Samir package fix.


Checking in packager/packages-unix;
/cvsroot/mozilla/xpinstall/packager/packages-unix,v  <--  packages-unix
new revision: 1.209.2.3; previous revision: 1.209.2.2
done
Checking in packager/packages-win;
/cvsroot/mozilla/xpinstall/packager/packages-win,v  <--  packages-win
new revision: 1.207.2.5; previous revision: 1.207.2.4
done
Checking in packager/unix/config.it;
/cvsroot/mozilla/xpinstall/packager/unix/config.it,v  <--  config.it
new revision: 1.20.2.4; previous revision: 1.20.2.3
done
Checking in packager/windows/config.it;
/cvsroot/mozilla/xpinstall/packager/windows/config.it,v  <--  config.it
new revision: 1.103.2.6; previous revision: 1.103.2.5
done
Checking in packager/windows/mail.jst;
/cvsroot/mozilla/xpinstall/packager/windows/mail.jst,v  <--  mail.jst
new revision: 1.37.2.4; previous revision: 1.37.2.3
done
Checking in packager/windows/xpcom.jst;
/cvsroot/mozilla/xpinstall/packager/windows/xpcom.jst,v  <--  xpcom.jst
new revision: 1.17.2.4; previous revision: 1.17.2.3
done
Checking in public/nsIXPINotifier.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPINotifier.idl,v  <--  nsIXPINotifier.idl
new revision: 1.6.2.4; previous revision: 1.6.2.3
done
Checking in src/Makefile.in;
/cvsroot/mozilla/xpinstall/src/Makefile.in,v  <--  Makefile.in
new revision: 1.60.2.4; previous revision: 1.60.2.3
done
Checking in src/ScheduledTasks.cpp;
/cvsroot/mozilla/xpinstall/src/ScheduledTasks.cpp,v  <--  ScheduledTasks.cpp
new revision: 1.40.2.4; previous revision: 1.40.2.3
done
Checking in src/ScheduledTasks.h;
/cvsroot/mozilla/xpinstall/src/ScheduledTasks.h,v  <--  ScheduledTasks.h
new revision: 1.15.2.4; previous revision: 1.15.2.3
done
Checking in src/gdiff.h;
/cvsroot/mozilla/xpinstall/src/gdiff.h,v  <--  gdiff.h
new revision: 1.4.36.4; previous revision: 1.4.36.3
done
Checking in src/makefile.win;
/cvsroot/mozilla/xpinstall/src/makefile.win,v  <--  makefile.win
new revision: 1.51.2.4; previous revision: 1.51.2.3
done
Checking in src/nsInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstall.cpp,v  <--  nsInstall.cpp
new revision: 1.197.2.5; previous revision: 1.197.2.4
done
Checking in src/nsInstall.h;
/cvsroot/mozilla/xpinstall/src/nsInstall.h,v  <--  nsInstall.h
new revision: 1.87.2.4; previous revision: 1.87.2.3
done
Checking in src/nsInstallBitwise.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallBitwise.cpp,v  <--  nsInstallBitwise.cpp
new revision: 1.2.114.4; previous revision: 1.2.114.3
done
Checking in src/nsInstallBitwise.h;
/cvsroot/mozilla/xpinstall/src/nsInstallBitwise.h,v  <--  nsInstallBitwise.h
new revision: 1.2.114.4; previous revision: 1.2.114.3
done
Checking in src/nsInstallExecute.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallExecute.cpp,v  <--  nsInstallExecute.cpp
new revision: 1.25.22.5; previous revision: 1.25.22.4
done
Checking in src/nsInstallFile.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallFile.cpp,v  <--  nsInstallFile.cpp
new revision: 1.63.2.4; previous revision: 1.63.2.3
done
Checking in src/nsInstallFileOpItem.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallFileOpItem.cpp,v  <-- 
nsInstallFileOpItem.cpp
new revision: 1.60.16.5; previous revision: 1.60.16.4
done
Checking in src/nsInstallFileOpItem.h;
/cvsroot/mozilla/xpinstall/src/nsInstallFileOpItem.h,v  <--  nsInstallFileOpItem.h
new revision: 1.20.36.4; previous revision: 1.20.36.3
done
Checking in src/nsInstallFolder.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallFolder.cpp,v  <--  nsInstallFolder.cpp
new revision: 1.59.2.4; previous revision: 1.59.2.3
done
Checking in src/nsInstallFolder.h;
/cvsroot/mozilla/xpinstall/src/nsInstallFolder.h,v  <--  nsInstallFolder.h
new revision: 1.15.2.4; previous revision: 1.15.2.3
done
Checking in src/nsInstallLogComment.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallLogComment.cpp,v  <-- 
nsInstallLogComment.cpp
new revision: 1.4.24.4; previous revision: 1.4.24.3
done
Checking in src/nsInstallPatch.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallPatch.cpp,v  <--  nsInstallPatch.cpp
new revision: 1.57.2.4; previous revision: 1.57.2.3
done
Checking in src/nsInstallUninstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstallUninstall.cpp,v  <--  nsInstallUninstall.cpp
new revision: 1.21.2.4; previous revision: 1.21.2.3
done
Checking in src/nsJSFile.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSFile.cpp,v  <--  nsJSFile.cpp
new revision: 1.29.16.4; previous revision: 1.29.16.3
done
Checking in src/nsJSFileSpecObj.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSFileSpecObj.cpp,v  <--  nsJSFileSpecObj.cpp
new revision: 1.4.66.4; previous revision: 1.4.66.3
done
Checking in src/nsJSInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstall.cpp,v  <--  nsJSInstall.cpp
new revision: 1.99.2.4; previous revision: 1.99.2.3
done
Checking in src/nsJSWinProfile.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSWinProfile.cpp,v  <--  nsJSWinProfile.cpp
new revision: 1.8.2.4; previous revision: 1.8.2.3
done
Checking in src/nsJSWinReg.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSWinReg.cpp,v  <--  nsJSWinReg.cpp
new revision: 1.13.2.4; previous revision: 1.13.2.3
done
Checking in src/nsLoggingProgressNotifier.cpp;
/cvsroot/mozilla/xpinstall/src/nsLoggingProgressNotifier.cpp,v  <-- 
nsLoggingProgressNotifier.cpp
new revision: 1.38.2.5; previous revision: 1.38.2.4
done
Checking in src/nsSoftwareUpdate.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.cpp,v  <--  nsSoftwareUpdate.cpp
new revision: 1.93.2.4; previous revision: 1.93.2.3
done
Checking in src/nsSoftwareUpdateRun.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp,v  <-- 
nsSoftwareUpdateRun.cpp
new revision: 1.74.2.4; previous revision: 1.74.2.3
done
Checking in src/nsTopProgressNotifier.cpp;
/cvsroot/mozilla/xpinstall/src/nsTopProgressNotifier.cpp,v  <-- 
nsTopProgressNotifier.cpp
new revision: 1.16.2.4; previous revision: 1.16.2.3
done
Checking in src/nsUpdateNotification.cpp;
/cvsroot/mozilla/xpinstall/src/nsUpdateNotification.cpp,v  <-- 
nsUpdateNotification.cpp
new revision: 1.4.14.4; previous revision: 1.4.14.3
done
Checking in src/nsWinProfile.cpp;
/cvsroot/mozilla/xpinstall/src/nsWinProfile.cpp,v  <--  nsWinProfile.cpp
new revision: 1.12.16.4; previous revision: 1.12.16.3
done
Checking in src/nsWinProfile.h;
/cvsroot/mozilla/xpinstall/src/nsWinProfile.h,v  <--  nsWinProfile.h
new revision: 1.5.36.4; previous revision: 1.5.36.3
done
Checking in src/nsWinProfileItem.cpp;
/cvsroot/mozilla/xpinstall/src/nsWinProfileItem.cpp,v  <--  nsWinProfileItem.cpp
new revision: 1.14.16.4; previous revision: 1.14.16.3
done
Checking in src/nsWinProfileItem.h;
/cvsroot/mozilla/xpinstall/src/nsWinProfileItem.h,v  <--  nsWinProfileItem.h
new revision: 1.10.36.4; previous revision: 1.10.36.3
done
Checking in src/nsWinReg.cpp;
/cvsroot/mozilla/xpinstall/src/nsWinReg.cpp,v  <--  nsWinReg.cpp
new revision: 1.20.2.4; previous revision: 1.20.2.3
done
Checking in src/nsWinReg.h;
/cvsroot/mozilla/xpinstall/src/nsWinReg.h,v  <--  nsWinReg.h
new revision: 1.12.2.4; previous revision: 1.12.2.3
done
Checking in src/nsWinRegItem.cpp;
/cvsroot/mozilla/xpinstall/src/nsWinRegItem.cpp,v  <--  nsWinRegItem.cpp
new revision: 1.19.16.4; previous revision: 1.19.16.3
done
Checking in src/nsWinRegItem.h;
/cvsroot/mozilla/xpinstall/src/nsWinRegItem.h,v  <--  nsWinRegItem.h
new revision: 1.13.24.4; previous revision: 1.13.24.3
done
Checking in src/nsXPITriggerInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.h,v  <--  nsXPITriggerInfo.h
new revision: 1.14.16.4; previous revision: 1.14.16.3
done
Checking in src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.103.2.5; previous revision: 1.103.2.4
done
Checking in stub/nsStubNotifier.cpp;
/cvsroot/mozilla/xpinstall/stub/nsStubNotifier.cpp,v  <--  nsStubNotifier.cpp
new revision: 1.10.2.4; previous revision: 1.10.2.3
done
Checking in stub/xpistub.cpp;
/cvsroot/mozilla/xpinstall/stub/xpistub.cpp,v  <--  xpistub.cpp
new revision: 1.44.2.4; previous revision: 1.44.2.3
done
dougt relanded Dan's patch, along with the Pink's mac fix and the Samir's
packaging fix. Marking as fixed1.0.0.
Keywords: fixed1.0.0
I verified branch installation on Simplified Chinese win2k, now path include
native characters can be processed correctly.
Thanks for testing this one out Shanjian.

ruixu - can you mark this bug as verified1.0.0?
My verification could not replace QA's verification. ruixu, please verify this
with today's branch build before you mark it verified. 
Verified fixed with branch build 2002-05-07 on all support platforms (SC XP, 
SC 2K, SC NT4, SC Me, SC 98SE, SC 95OSR2, JA XP, JA 2K, JA NT4, JA Me, JA 98SE, 
JA 95OSR2). 
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
removing item for this bug from the release notes since the bug is marked
fixed. If this is in error, please make a note in the release notes bug for
the current milestone. As of today, this is bug 133795. In a ew weeks, it 
will be some other bug.
This bug isn't fixed completely. See bug 153669.
ruixu: this is NOT fixed on the trunk, only on the branch. Please leave
resolving bugs to the engineer doing the check-in
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Bug 153669 has been marked as a duplicate of this bug. ***
Removing adt1.0.0+, as tis bug has been reopened.
Keywords: adt1.0.0+
Whiteboard: [adt1] [hitlist] [m5+] [ETA 05/06] → [adt1 RTM] [hitlist] [m5+] [ETA 05/25]
doh! made changes without seeing dveditz comments. adding back adt1.0.0+
Keywords: adt1.0.0+
Whiteboard: [adt1 RTM] [hitlist] [m5+] [ETA 05/25] → [adt1 RTM] [hitlist] [m5+] [ETA 05/25][not fixed on trunk]
2002120408-trunk/Win2k-J still has the same problem.
Why the patch is not checked into the trunk build?
Adding to dependencies of meta bug 157673 so that this will get considered for
the trunk by internationalization triage.
Blocks: 157673
Whiteboard: [adt1 RTM] [hitlist] [m5+] [ETA 05/25][not fixed on trunk] → [adt1 RTM] [ETA 05/25][not fixed on trunk]
over to me to do the merge from branch -> trunk.
Assignee: dveditz → ssu
Status: REOPENED → NEW
Installer triage team: nsbeta1+/adt1
Whiteboard: [adt1 RTM] [ETA 05/25][not fixed on trunk] → [adt1][not fixed on trunk]
Attached patch updated trunk patch v1.1 (obsolete) — Splinter Review
this patch is a combined patch from the following two previously attached
patches merged with the current trunk:
  http://bugzilla.mozilla.org/attachment.cgi?id=82068&action=view
  http://bugzilla.mozilla.org/attachment.cgi?id=82190&action=view

testing to make sure bug has not regressed...
QA Contact: ruixu → ylong
ignore the patch.  it's already working on the trunk.

however the other problems also discussed in this bug, correspond to the
following two bugs:

  bug 125106 - Two garbage named folders were created in Documents and
               Settings | All Users after installation
  bug 125104 - JA WinXP and Win2K: Cannot uninstall Mozilla completely

closing this bug as fixed (on the trunk).
Status: NEW → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
oops, instead of bug 125106, I meant bug 192166.
Mark as verified - icons and program folder can be installed on latest trunk build.
Status: RESOLVED → VERIFIED
If this patch didn't land on the trunk then it's not fixed :-(

Lots of problems only surfaced when you used non-Wester machine locales, or intl
chars in various directories we used. The dataloss in various places can be
subtle. Also check the checkin comments on the 1.0 branch, this patch included
several sub-bugs that appear not to be marked in the dependency tree of this bug.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 187979
Changing summary to reflect huge scope of the patch. The original summary
described a problem that uncovered the fact that nsIFile changed a behavior we
were counting on, leading to the need for an XPInstall-wide fix (see comment #26).
No longer blocks: 187979
Summary: Cannot install desktop icon and program folder properly on localized Windows → general nsIFile converter change hosed XPInstall intl filenames
Blocks: 187979
Attached patch patch v1.2 (obsolete) — Splinter Review
This is a patch that is merged with the trunk.	I've ran some quick xpinstall
smoketests, and everything seems to be working fine.  I'll attach my own
getFolder test .xpi file that tests the newly added win32 folders (with this
patch).
Attachment #114155 - Attachment is obsolete: true
This test .xpi file will test the newly supported folders under Win32.	Here is
a sample output of the .xpi file when run under WinXP:

---------------------------------------------------------------------------
file:///C:/download/temp/getFolderText.xpi  --	04/07/2003 17:46:16
---------------------------------------------------------------------------

  GetFolderTest (version 1.0.1.1)
  -------------

  **   Win32 folders
  ** Win System 	  : D:\WINDOWS\SYSTEM32\
  ** Windows		  : D:\WINDOWS\
  ** Temporary		  : D:\DOCUME~1\[user]\LOCALS~1\TEMP\
  **
  **   New Win32 folders
  ** Win Desktop	  : D:\DOCUMENTS AND SETTINGS\[user]\DESKTOP\
  ** Win Desktop Common   : D:\DOCUMENTS AND SETTINGS\ALL USERS\DESKTOP\
  ** Win StartMenu	  : D:\DOCUMENTS AND SETTINGS\[user]\START
MENU\PROGRAMS\STARTUP\
  ** Win StartMenu Common : D:\DOCUMENTS AND SETTINGS\ALL USERS\START MENU\
  ** Win Programs	  : D:\DOCUMENTS AND SETTINGS\[user]\START
MENU\PROGRAMS\
  ** Win Programs Common  : D:\DOCUMENTS AND SETTINGS\ALL USERS\START
MENU\PROGRAMS\
  ** Win Startup	  : D:\DOCUMENTS AND SETTINGS\[user]\START
MENU\PROGRAMS\STARTUP\
  ** Win Startup Common   : D:\DOCUMENTS AND SETTINGS\ALL USERS\START MENU\
  ** Win AppData	  : D:\DOCUMENTS AND SETTINGS\[user]\APPLICATION DATA\
  ** Win Program Files	  : D:\Program Files\
  ** Win Common Files	  : D:\Program Files\Common Files\

  Install completed successfully  --  04/07/2003 17:46:16
Attachment #119753 - Flags: review?(dveditz)
There's a test build with patch v1.2 applied delivered to here:
  ftp://ftp.mozilla.org/pub/mozilla/nightly/2003-04-14-16-trunk

The internal build is here:
 
http://sweetlou.mcom.com/products/client/seamonkey/windows/32bit/x86/2003-04-14-16-trunk

can someone try this out and see if it fixes this bug?
Status: REOPENED → ASSIGNED
Here is results for mozilla 04-14-16 trunk build ( I didn't try Netscape trunk
build because I can not remove the Netscape icons due another bug) on WinXP-JA:
Mozilla can be installed in desktop icon and start up menu.  However, after
uninstall, the mozilla desktop icon and start up menu still stay.
The uninstall problem is bug 143514 and possible also bug 137656.
Because the bug summary has been changed, I'm not clear with what we try to fix
now, what's the difference will make by checking in the patch1.2? 

I remember I can see the icons since a while ago and just tried a 04-14-08 trunk
build (before patch1.2 checked-in), I had the same behavior.
Attached patch patch v1.3 (obsolete) — Splinter Review
fixed some string conversion problems and also added fix for bug 187979.
Attachment #119753 - Attachment is obsolete: true
Attachment #119753 - Flags: review?(dveditz)
Comment on attachment 121144 [details] [diff] [review]
patch v1.3

since I was responsible for bug 100676, I figured I'd help out with a review...

+	 // If we clone an nsIFile, and move the clone, the FSRef of the
*original*
+	 // file is not what you would expect - it points to the moved file.
This
+	 // is despite the fact that the two FSRefs are independent objects.
Until
+	 // the OS X file impl is changed to not use FSRefs, need to do this.

please reference bug 200024 in the comment so readers can easily track this (I
know you're just re-indenting..)

-	 leafname.Append("old");
+	 leafname.Append(NS_LITERAL_STRING("old"));

you can use AppendNative(NS_LITERAL_CSTRING(...)) when you're just appending
ascii strings


+	 res = folder->Init(NS_ConvertASCIItoUCS2("file:///"), fname);

here, use NS_LITERAL_STRING() - if this api takes a const nsString&, change it
to take a const nsAString& (or const nsAFlatString& if you need access to
.get())



+	 nsCOMPtr<nsILocalFile> packageDir;
+	 NS_NewNativeLocalFile( nsDependentCString(szRegPackagePath), // native
path
+				PR_FALSE, getter_AddRefs(packageDir) );

nit: spacing is funny here - I don't see this style elsewhere in this file
(spaces after parens, etc)


-      temp = NS_ConvertUCS2toUTF8(*mParams);
+      temp = NS_LossyConvertUCS2toASCII(mParams);

yikes - are mParams guaranteed to be ASCII? (should this be a native string
converison? I don't think we have a native converter available (maybe UCS2toFS
is available?) but at least put a comment here for why this is safe)

-	 temp.Append(NS_LITERAL_CSTRING("\\") +
NS_LossyConvertUCS2toASCII(*mDescription));
+	 temp.Append(NS_LITERAL_CSTRING("\\") +
NS_LossyConvertUCS2toASCII(mDescription));

same here - why is this safe? Comments should explain

-    arguments = ToNewCString(*mParams);
+    arguments = ToNewCString(mParams);

there's a implicit lossy conversion happening here - explain why this is safe
in a comment

     mSrc->GetParent(getter_AddRefs(parent));
+    mSrc->GetParent(getter_AddRefs(newDirName));
+    newDirName->Append(*mStrTarget);
     ret = newDirName->MoveTo(parent, leafName);

since you aren't using 'parent' until the last statement, move the 1st
GetParent() just before it (for readability, otherwise it looks redundant)

+#else
+		     // XXX: Need to fix non-Windows conversions too
+		     tmpPath.Append(NS_LossyConvertUCS2toASCII(aRelativePath));
+#endif

can you fix this now, or add a comment as to what you're blocked on?

+		 BYTE path[ 2 * _MAX_PATH ] = { 0 };

where does the "2 *" come from? is this unicode? should this be 
"sizeof(PRUnichar) * " instead? (or if you're just being safe, add a comment)

+    // safe compare because all strings in DirectoryTable are ASCII
+    if ( name.EqualsIgnoreCase(DirectoryTable[i].directoryName) )

nice! (the comment I mean :)

+    // Since bug 100676 was fixed we should never get here

should we NS_ASSERTION rather than NS_WARNING then? (the assertion will
actually throw up a debug dialog on windows, which is always good)

+	     leafName.Right(ext, (leafName.Length() - i) );
+	     leafName.Left(fName, (leafName.Length() - (leafName.Length() -
i)));
+	     leafName.Assign(fName);
+	     leafName.Append(tmpName);
+	     leafName.Append(ext);

just a comment explaining what all this string manipulation is...


*whew* - I'm only halfway throught this patch - this is fantastic cleanup
though! All around great stuff...

(for my own reference, I stopped here:
Index: src/nsInstallUninstall.cpp
)
Comment on attachment 121144 [details] [diff] [review]
patch v1.3

r=ssu
Attachment #121144 - Flags: review+
Comment on attachment 121144 [details] [diff] [review]
patch v1.3

agh, wrong bug. sorry.
Attachment #121144 - Flags: review+
Attached patch patch v1.4 (obsolete) — Splinter Review
updated patch given alecf's comments.

I also fixed up:
 // XXX: Need to fix non-Windows conversions too

it now calls:
  NS_CopyUnicodeToNative()
  NS_CopyNativeToUnicode()

instead of:
  UCS2toFS()
  FStoUCS2()

The copy functions are part of xpcom which basically does the same thing, but
it's cross platform.

The win32 native installer works with this patch.  I've ran some regression
xpinstall trigger tests also, and it looks good under win32.  I'll be testing
linux and OSX next.
Attachment #121144 - Attachment is obsolete: true
linux and OSX work fine with the patch too.

requesting blocking1.4b because the ammount of changes is not suitable for 1.4f.
 We need to get this tested in 1.4b.
Flags: blocking1.4b?
Attachment #121928 - Flags: superreview?(alecf)
Attachment #121928 - Flags: review?(dveditz)
Comment on attachment 121928 [details] [diff] [review]
patch v1.4

this is great cleanup. 
in nsWinReg::NativeGetValueString(const nsString& aSubkey, const nsString&
aValname, nsString* aReturn)

I don't think you need the const char* cast:


+	     nsCAutoString cstrValue((const char*)valbuf);
+	     nsAutoString value;
+	     if (NS_SUCCEEDED( NS_CopyNativeToUnicode(cstrValue, value) ) )
+	       aReturn->Assign(value);
+	     else
+	       result = nsInstall::UNEXPECTED_ERROR;
+

If you do need it for some reason (the unsigned-ness maybe?) then use a
C++-style cast and comment - most likely all you should need is a
NS_STATIC_CAST()

The code looks good, I'll leave it up to dveditz to verify that we're doing the
right conversions int he right place.

sr=alecf
Attachment #121928 - Flags: superreview?(alecf) → superreview+
Comment on attachment 121928 [details] [diff] [review]
patch v1.4

Looks great! Need a couple of minor changes before I can r=


> nsInstall::GetComponentFolder(const nsString& aComponentName, const nsString& aSubdirectory, nsInstallFolder** aNewFolder)
[...]
>+        nsAutoString fname;
>+
>+        if(!aSubdirectory.IsEmpty())
>+          componentIFile->Append(aSubdirectory);

That won't work: if aSubdirectory contains more than just a filename it'll have
unix-style slashes on all platforms (the XPInstall script convention) which
will fail on non-unix platforms. Come to think of it, plain Append accepts only
a single node and will reject slashes everywhere.

>+        componentIFile->GetPath(fname);
>+        res = folder->Init(NS_LITERAL_STRING("file:///"), fname);

And using "file:///" is ugly and error prone. Script authors may have no choice
on occassion, but in C++ land we've always got better ways. nsInstallFolder
should support a form of Init that takes an nsIFile, so you can reduce the
above (after adding back parts I skipped) to

	  res = folder->Init(componentIFile, aSubdirectory);

I haven't looked to see if the nsInstallFolder part of the original patch
survived, but it should for just this case.


>         if (!pKey.IsEmpty() && !pKey.IsEmpty())

I believe this should be...

	  if (!pKey.IsEmpty() && !pVal.IsEmpty())

...no point in checking pKey twice.

>         {
>             JSString* propValJSStr = JS_NewUCStringCopyZ(cx, NS_REINTERPRET_CAST(const jschar*, pVal.get()));
>             jsval propValJSVal = STRING_TO_JSVAL(propValJSStr);
>-            JS_SetProperty(cx, res, pKey.get(), &propValJSVal);
>+            JS_SetUCProperty(cx, res, NS_REINTERPRET_CAST(const jschar*, pKey.get()), pKey.Length(), &propValJSVal);
>         }

I think the original was better: pKey is a nsCString, and a reinterpret cast
(basically C-style "I know what I'm doing, dammit") doesn't magically make it
Unicode/wide (as jschar* is). The result would be amusingly garbled plus read
into someone else's memory.

Looking back, when this change was made in the original patch pKey was a
widestring, but since nsIPersistentProperties2.idl was changed so that key is
now narrow making this change unnecessary. The dangers of reinterpret cast
exposed once again.

But wait, there's more: key is really Utf8 if you check the .idl, so to truly
preserve the intended key you'd have to use NS_ConvertUTF8toUCS2() first, and
then use the unicode version SetUCProperty as above. Ick -- do we really expect
non-ascii keys in a resource file?

>       if(err != 0)
>-        aShortPathName.AssignWithConversion(nativeShortPathNameTmp);
>+      {
>+        // if err is 0, it's not a buffer size problem.  It's something else unexpected.
>+        nsCAutoString castrNativeShortPathNameTmp(nativeShortPathNameTmp);
>+        NS_CopyNativeToUnicode(castrNativeShortPathNameTmp, unicodePath);
>+      }

You could save the nsCAutoString copy and potential reallocation by using
nsDependentCString I think:

  NS_CopyNativeToUnicode(nsDependentCString(nativeShortPathNameTmp),
unicodePath);

>+      nsCAutoString castrNativeShortPathName(nativeShortPathName);
>+      NS_CopyNativeToUnicode(castrNativeShortPathName, unicodePath);

same here

>         nsCAutoString unboundFile;
>         uniqueSrcFile->GetNativePath(unboundFile);
> 
>         if (su_unbind((char*)realfile.get(), (char*)unboundFile.get()))  //
>         {
>-            realfile = unboundFile;
>+            // un-binding worked, save the tmp name for later
>+            uniqueSrcFile->GetPath(NS_ConvertUTF8toUCS2(realfile));

Can't call a "Get..." function with a NS_Convert temp variable. Plus UTF8 is
wrong, we want the native charset.

	     uniqueSrcFile->GetNativePath(realfile);

>     nsCString name; name.AssignWithConversion(UIPackageName);
>+    nsCString version; version.AssignWithConversion(aVersion);

The name conversion could use NS_CopyUnicodeToNative() instead
of the lossy ascii conversion. version shouldn't matter--it's
all ascii--but it wouldn't hurt either. If you leave it, add a
comment to that effect.
Attachment #121928 - Flags: review?(dveditz) → review-
Attached patch patch v1.5Splinter Review
Attachment #121928 - Attachment is obsolete: true
Comment on attachment 122067 [details] [diff] [review]
patch v1.5

r=dveditz -- thanks for the cleanup and huge merge job.

This should definitely go into 1.4b to get wider testing, though it has
essentially been in my tree for quite a while without problems.

carrying over alecf's sr=, requesting 1.4b approval
Attachment #122067 - Flags: superreview+
Attachment #122067 - Flags: review+
Attachment #122067 - Flags: approval1.4b?
Comment on attachment 122067 [details] [diff] [review]
patch v1.5

a=sspitzer
Attachment #122067 - Flags: approval1.4b? → approval1.4b+
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
The programe name and icons are installed fined with 04-30 on WinXP, aslo the
problem in bug 192166 was fixed on both Win2000-JA and WinXP-JA.

Mark this one as verified.
Status: RESOLVED → VERIFIED
Flags: blocking1.4b?
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.