125106 - general nsIFile converter change hosed XPInstall intl filenames

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect, P1)

Description

[Rui Xu]

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.

Comment 1

[Syd Logan]

need to see the install log please, might help us understand the problem.

Comment 2

[Rui Xu]

Attachment: install log file

An install log file is attached, please let me know if need more help. Thanks.

Comment 3

[Grace Bush]

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

Comment 4

[Rui Xu]

I cannot repro this bug on EN XP as well.
It only can be reproduced on localized XP, e.g. JA XP, SC XP.

Comment 5

[Grace Bush]

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

Comment 6

[Rui Xu]

Attachment: before installation <-> after installation

FYI, in some localized OS, The folder name "Start Menu" and "Desktop" are
localized, they are not in English.

Comment 7

[Daniel Veditz [:dveditz]]

over to Dave

Comment 8

[Ying-Lin Xia]

Info in bugscape 12401 could be helpful. Looks like same issue here.

Comment 9

[Syd Logan]

nsbeta1- per adt triage

Comment 10

[Rui Xu]

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.

Comment 11

[Ying-Lin Xia]

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. 

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Ying-Lin Xia]

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.

Comment 14

[rchen]

Dan, do you mind posting the broken code? We will do as much as we can to fix 
it.

Comment 15

[Ying-Lin Xia]

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?

Comment 16

[rchen]

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. 

Comment 17

[ji]

Renominated for nsbeta1, since it's a serious regression from the previous release.

Comment 18

[Rui Xu]

> 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.

Comment 19

[rchen]

Hmm, the checkin is between 20020130 and 20020212. It shouldn't be hard to find 
out what causes it.

Comment 20

[Daniel Veditz [:dveditz]]

If you spot any candidates please let me know. None of the xpinstall changes
during that time frame would affect this functionality.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fxpinstall&file=&filetype=match&who=&whotype=match&hours=2&date=explicit&mindate=01%2F30%2F2002&maxdate=02%2F12%2F2002&cvsroot=%2Fcvsroot

Comment 21

[Michele Carlson]

changing to nsbeta1+

Comment 22

[dprice (gone)]

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

Comment 23

[Rui Xu]

It also can be reproducible with 20020202 trunk build, the checkin is between 
20020130 and 20020202. Thanks.

Comment 24

[rchen]

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);

Comment 25

[Katsuhiko Momoi]

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.

Comment 26

[Daniel Veditz [:dveditz]]

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.

Comment 27

[Michele Carlson]

adding to whiteboard adt1
cc: richie close

Comment 28

[rchen]

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.

Comment 29

[rchen]

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));  

Comment 30

[rchen]

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?

Comment 31

[Daniel Veditz [:dveditz]]

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

Comment 32

[Rose Smyth]

adding pgorman

Comment 33

[James "Kovu" Russell]

*** Bug 125958 has been marked as a duplicate of this bug. ***

Comment 34

[Michele Carlson]

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!

Comment 35

[Michele Carlson]

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

Comment 36

[Daniel Veditz [:dveditz]]

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

Comment 37

[Gashu]

Attachment: Directory names on JA Windows(SHIFT JIS)

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.

Comment 38

[Gashu]

Is this also reproducable on Chinese Windows?

Comment 39

[Rui Xu]

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.

Comment 40

[Daniel Veditz [:dveditz]]

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.

Comment 41

[Frank Tang]

roy yokoyama, please help dveditz

Comment 42

[Roy Yokoyama]

let's see what I can do.

Comment 43

[Daniel Veditz [:dveditz]]

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.

Comment 44

[Roy Yokoyama]

Let me know if you need my help, dveditz.

Comment 45

[rchen]

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. 

Comment 46

[Michael Buckland]

adding metabug blockage

Comment 47

[Michael Buckland]

Taking this on my hitlist.  As rchen pointed out below, we need an ETA for the
langing of a fix.  Dan?

Comment 48

[Frank Tang]

>roy yokoyama, please help dveditz
[e-mail]: 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)

Comment 49

[Gashu]

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

Comment 50

[Michael Buckland]

ADT talked to Dan today.  He said he can land a fix by 4/12, then needs to go
thru  review process.

Comment 51

[Jaime Rodriguez, Jr.]

04/12 was last friday. did you mean 04/17?

Comment 52

[Gashu]

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.

Comment 53

[Michael Buckland]

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?

Comment 54

[scottputterman]

How is the patch going on this?  Any new ETA since 4/26 has passed?

Comment 55

[Gashu]

Attachment: 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.

Comment 56

[scottputterman]

Changing eta to today (4/29) based on conversation with dveditz.

Comment 57

[Daniel Veditz [:dveditz]]

*** Bug 141099 has been marked as a duplicate of this bug. ***

Comment 58

[Daniel Veditz [:dveditz]]

*** Bug 140712 has been marked as a duplicate of this bug. ***

Comment 59

[scottputterman]

adding 4/30 to the eta based on today's conversation with Dan.

Comment 60

[Daniel Veditz [:dveditz]]

Attachment: Patch to fix charset/filepath bogosities

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

Comment 61

[Daniel Veditz [:dveditz]]

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.

Comment 62

[Daniel Veditz [:dveditz]]

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.

Comment 63

[Frank Tang]

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 64

[Alec Flett]

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.

Comment 65

[nhottanscp]

Syd or dveditz, could you create a test build so IQA can try it before checking
in to the branch?

Comment 66

[Curt Patrick (gone)]

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.

Comment 67

[Frank Tang]

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 68

[Doug Turner (:dougt)]

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

Comment 69

[scottputterman]

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.

Comment 70

[Daniel Veditz [:dveditz]]

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

Comment 71

[Frank Tang]

dan- can you just build it for us?

Comment 72

[Frank Tang]

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

Comment 73

[Daniel Veditz [:dveditz]]

Test build available to Netscape folks at
  ftp://[e-mail]/../../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.

Comment 74

[Daniel Veditz [:dveditz]]

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.

Comment 75

[Roy Yokoyama]

dan: I tried "ftp://[e-mail]/../../u/dveditz/125106.exe";
     but got "page doesnt exist" err in Chinese. No password prompt.
     What am I missing?

Comment 76

[Daniel Veditz [:dveditz]]

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.

Comment 77

[Roy Yokoyama]

Humm, the link is paste in #75 works now.  

Comment 78

[Roy Yokoyama]

Humm, the pasted link in #75 works now, but desktop icon and program folder
don't get created on WinXP-SC using 
"ftp://[e-mail]/../../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?

Comment 79

[Daniel Veditz [:dveditz]]

Attachment: patch merged to the trunk

branch patch merged with the trunk nsIFile changes from bug 129279

Comment 80

[Daniel Veditz [:dveditz]]

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!)

Comment 81

[Rui Xu]

Attachment: Error message when installing build 125106.exe

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.

Comment 82

[Rui Xu]

Attachment: The corrupted folders in 0502 trunk

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 83

[Doug Turner (:dougt)]

Comment on attachment 82068 [details] [diff] [review]
patch merged to the trunk

dougt

Comment 84

[Daniel Veditz [:dveditz]]

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.

Comment 85

[Rui Xu]

Attachment: install_wizard.log file

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.

Comment 86

[Daniel Veditz [:dveditz]]

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.

Comment 87

[Frank Tang]

ask shanjian to help on this as the highest priority.

Comment 88

[Daniel Veditz [:dveditz]]

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.

Comment 89

[rchen]

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"? 

Comment 90

[Rui Xu]

Attachment: A screen shot for problematic areas in installation

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.

Comment 91

[Rui Xu]

>> 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.

Comment 92

[Daniel Veditz [:dveditz]]

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.

Comment 93

[Jaime Rodriguez, Jr.]

Removing adt1.0.0, until we have a new patch with reviews.

Comment 94

[Daniel Veditz [:dveditz]]

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.

Comment 95

[Daniel Veditz [:dveditz]]

Attachment: diff between the original patch and the current patch

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.

Comment 96

[Daniel Veditz [:dveditz]]

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.

Comment 97

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 brnach. thanks!

Comment 98

[Jaime Rodriguez, Jr.]

ruixu/shanjian - can we get this verified on the branch asap today. thanks!

Comment 99

[Rui Xu]

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.

Comment 100

[Darin Fisher]

blocker bug http://bugscape.netscape.com/show_bug.cgi?id=14744 may be related to
these changes.

Comment 101

[Frank Tang]

darin- is 14744  linux only changes?

Comment 102

[Rui Xu]

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!

Comment 103

[Jaime Rodriguez, Jr.]

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.

Comment 104

[Samir Gehani]

Leaf backed this out of the branch yesterday.  Removing fixed1.0.0 keyword.

Comment 105

[Jaime Rodriguez, Jr.]

Leaf backed this out of the branch on Friday becaused it caused install failure
on Linux.  Removing adt1.0.0+ keyword.

Comment 106

[timeless]

i thought he backed it out because it didn't have approval

Comment 107

[Asa Dotzler [:asa]]

We need reviews and super-review here and we'll reconsider for RC2 (now that bug
142107 is fixed) 

Comment 108

[Doug Turner (:dougt)]

asa we will just reland what dan landed.  we already have r/sr/a for that, don't we?

Comment 109

[Jaime Rodriguez, Jr.]

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.

Comment 110

[chris hofmann]

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.

Comment 111

[Darin Fisher]

this patch looks good to me.

Comment 112

[Shanjian Li]

nsInstallFolder.cpp is the change to cope with localized CJK system installation. 
It looks fine to me from i18n standpoint. 

Comment 113

[Doug Turner (:dougt)]

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

Comment 114

[Jaime Rodriguez, Jr.]

dougt relanded Dan's patch, along with the Pink's mac fix and the Samir's
packaging fix. Marking as fixed1.0.0.

Comment 115

[Shanjian Li]

I verified branch installation on Simplified Chinese win2k, now path include
native characters can be processed correctly.

Comment 116

[Jaime Rodriguez, Jr.]

Thanks for testing this one out Shanjian.

ruixu - can you mark this bug as verified1.0.0?

Comment 117

[Shanjian Li]

My verification could not replace QA's verification. ruixu, please verify this
with today's branch build before you mark it verified. 

Comment 118

[Rui Xu]

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

Comment 119

[Dawn Endico]

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.

Comment 120

[Koike Kazuhiko]

This bug isn't fixed completely. See bug 153669.

Comment 121

[Daniel Veditz [:dveditz]]

ruixu: this is NOT fixed on the trunk, only on the branch. Please leave
resolving bugs to the engineer doing the check-in

Comment 122

[Daniel Veditz [:dveditz]]

*** Bug 153669 has been marked as a duplicate of this bug. ***

Comment 123

[Jaime Rodriguez, Jr.]

Removing adt1.0.0+, as tis bug has been reopened.

Comment 124

[Jaime Rodriguez, Jr.]

doh! made changes without seeing dveditz comments. adding back adt1.0.0+

Comment 125

[Hideo Oshima]

2002120408-trunk/Win2k-J still has the same problem.
Why the patch is not checked into the trunk build?

Comment 126

[Simon Montagu :smontagu]

Adding to dependencies of meta bug 157673 so that this will get considered for
the trunk by internationalization triage.

Comment 127

[Sean Su]

over to me to do the merge from branch -> trunk.

Comment 128

[Samir Gehani]

Installer triage team: nsbeta1+/adt1

Comment 129

[Sean Su]

Attachment: updated trunk patch v1.1

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...

Comment 130

[Sean Su]

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

Comment 131

[Sean Su]

oops, instead of bug 125106, I meant bug 192166.

Comment 132

[Yuying Long]

Mark as verified - icons and program folder can be installed on latest trunk build.

Comment 133

[Daniel Veditz [:dveditz]]

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.

Comment 134

[Daniel Veditz [:dveditz]]

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

Comment 135

[Sean Su]

Attachment: patch v1.2

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

Comment 136

[Sean Su]

Attachment: test .xpi file that will test getFolder

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

Comment 137

[Sean Su]

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?

Comment 138

[Yuying Long]

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.

Comment 139

[Sean Su]

The uninstall problem is bug 143514 and possible also bug 137656.

Comment 140

[Yuying Long]

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.

Comment 141

[Sean Su]

Attachment: patch v1.3

fixed some string conversion problems and also added fix for bug 187979.

Comment 142

[Alec Flett]

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 143

[Sean Su]

Comment on attachment 121144 [details] [diff] [review]
patch v1.3

r=ssu

Comment 144

[Sean Su]

Comment on attachment 121144 [details] [diff] [review]
patch v1.3

agh, wrong bug. sorry.

Comment 145

[Sean Su]

Attachment: patch v1.4

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.

Comment 146

[Sean Su]

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.

Comment 147

[Alec Flett]

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

Comment 148

[Daniel Veditz [:dveditz]]

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.

Comment 149

[Sean Su]

Attachment: patch v1.5

Comment 150

[Daniel Veditz [:dveditz]]

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

Comment 151

[(not reading, please use [e-mail] instead)]

Comment on attachment 122067 [details] [diff] [review]
patch v1.5

a=sspitzer

Comment 152

[Sean Su]

patch checked in.

Comment 153

[Yuying Long]

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.