bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

general nsIFile converter change hosed XPInstall intl filenames

VERIFIED FIXED in mozilla1.0

Status

SeaMonkey
Installer
P1
critical
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: Rui Xu, Assigned: Sean Su)

Tracking

({intl, regression})

Trunk
mozilla1.0
x86
Windows XP
intl, regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1][not fixed on trunk])

Attachments

(14 attachments, 4 obsolete attachments)

3.16 KB, text/plain
Details
114.84 KB, image/jpeg
Details
17 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+
Alec Flett
: superreview+
chris hofmann
: 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+
(not reading, please use seth@sspitzer.org instead)
: approval1.4b+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: intl, nsbeta1, regression
QA Contact: bugzilla → ruixu

Comment 1

17 years ago
need to see the install log please, might help us understand the problem.
Status: NEW → ASSIGNED
(Reporter)

Comment 2

17 years ago
Created attachment 69392 [details]
install log file

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

Comment 3

17 years ago
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
(Reporter)

Comment 4

17 years ago
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

17 years ago
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 
(?)
(Reporter)

Comment 6

17 years ago
Created attachment 73586 [details]
before installation <-> after installation

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

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla1.0
over to Dave
Assignee: dveditz → dprice
Status: ASSIGNED → NEW

Comment 8

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

Comment 9

17 years ago
nsbeta1- per adt triage
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla1.0 → ---
(Reporter)

Comment 10

17 years ago
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
(Reporter)

Updated

17 years ago
Severity: normal → critical

Comment 11

17 years ago
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.

Comment 13

17 years ago
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

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

Comment 15

17 years ago
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

17 years ago
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

17 years ago
Renominated for nsbeta1, since it's a serious regression from the previous release.
Keywords: nsbeta1- → nsbeta1
(Reporter)

Comment 18

17 years ago
> 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: nsbeta1 → nsbeta1-

Comment 19

17 years ago
Hmm, the checkin is between 20020130 and 20020212. It shouldn't be hard to find 
out what causes it.
(Reporter)

Updated

17 years ago
Keywords: nsbeta1- → nsbeta1

Comment 21

17 years ago
changing to nsbeta1+
Keywords: nsbeta1 → nsbeta1+

Comment 22

17 years ago
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
(Reporter)

Comment 23

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

Comment 24

17 years ago
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

17 years ago
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.

Comment 27

17 years ago
adding to whiteboard adt1
cc: richie close
Whiteboard: adt1

Comment 28

17 years ago
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

17 years ago
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

17 years ago
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

Comment 32

17 years ago
adding pgorman

Updated

17 years ago
Whiteboard: adt1 → [adt1]

Comment 33

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

Comment 34

17 years ago
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

17 years ago
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

Comment 37

17 years ago
Created attachment 78202 [details]
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

17 years ago
Is this also reproducable on Chinese Windows?
(Reporter)

Comment 39

17 years ago
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.

Comment 41

17 years ago
roy yokoyama, please help dveditz

Comment 42

17 years ago
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.

Comment 44

17 years ago
Let me know if you need my help, dveditz.

Comment 45

17 years ago
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

17 years ago
adding metabug blockage
Blocks: 136384

Comment 47

17 years ago
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

Updated

17 years ago
Whiteboard: [adt1] ETA UNKNOWN → [adt1] ETA UNKNOWN[hitlist]

Comment 48

17 years ago
>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)

Comment 49

17 years ago
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

17 years ago
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]

Comment 51

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

Updated

17 years ago
Whiteboard: [adt1] ETA 4/19[hitlist] → [adt1] ETA 4/19[hitlist] [m5+]

Comment 52

17 years ago
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

17 years ago
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+]

Comment 54

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

Comment 55

16 years ago
Created attachment 81316 [details]
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.

Updated

16 years ago
Attachment #81316 - Attachment mime type: application/octet-stream → text/plain

Comment 56

16 years ago
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. ***

Comment 59

16 years ago
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+]
Created attachment 81829 [details] [diff] [review]
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
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.

Updated

16 years ago
Whiteboard: [adt1] ETA 4/30[hitlist] [m5+] need reviews → [adt1] [hitlist] [m5+] [ETA 4/30] need reviews

Comment 63

16 years ago
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

16 years ago
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+

Comment 65

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

Comment 66

16 years ago
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

16 years ago
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

16 years ago
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+

Comment 69

16 years ago
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

Comment 71

16 years ago
dan- can you just build it for us?

Comment 72

16 years ago
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.

Comment 75

16 years ago
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

Comment 77

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

Comment 78

16 years ago
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?
Created attachment 82068 [details] [diff] [review]
patch merged to the trunk

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

Comment 81

16 years ago
Created attachment 82080 [details]
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.
(Reporter)

Comment 82

16 years ago
Created attachment 82086 [details]
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

16 years ago
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.
(Reporter)

Comment 85

16 years ago
Created attachment 82096 [details]
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.
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

16 years ago
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.

Comment 89

16 years ago
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"? 

(Reporter)

Comment 90

16 years ago
Created attachment 82150 [details]
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.
(Reporter)

Comment 91

16 years ago
>> 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.

Comment 93

16 years ago
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.
Created attachment 82190 [details]
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.
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

Comment 97

16 years ago
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=]

Comment 98

16 years ago
ruixu/shanjian - can we get this verified on the branch asap today. thanks!
(Reporter)

Comment 99

16 years ago
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

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

Comment 101

16 years ago
darin- is 14744  linux only changes?
(Reporter)

Comment 102

16 years ago
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.

Comment 104

16 years ago
Leaf backed this out of the branch yesterday.  Removing fixed1.0.0 keyword.
Keywords: fixed1.0.0

Updated

16 years ago
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+

Comment 106

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

Updated

16 years ago
Keywords: adt1.0.0

Comment 107

16 years ago
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.0 → adt1.0.0+
Priority: -- → P1
Whiteboard: [adt1] [hitlist] [m5+] [ETA 05/06] [Needs New Patch] → [adt1] [hitlist] [m5+] [ETA 05/06]
Target Milestone: --- → mozilla1.0

Comment 110

16 years ago
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.

Updated

16 years ago
Attachment #81829 - Flags: approval+

Comment 111

16 years ago
this patch looks good to me.

Comment 112

16 years ago
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

Comment 115

16 years ago
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?

Comment 117

16 years ago
My verification could not replace QA's verification. ruixu, please verify this
with today's branch build before you mark it verified. 
(Reporter)

Comment 118

16 years ago
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
Last Resolved: 16 years ago
Keywords: fixed1.0.0 → verified1.0.0
Resolution: --- → FIXED

Comment 119

16 years ago
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

16 years ago
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]

Comment 125

16 years ago
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

Updated

16 years ago
Whiteboard: [adt1 RTM] [hitlist] [m5+] [ETA 05/25][not fixed on trunk] → [adt1 RTM] [ETA 05/25][not fixed on trunk]
(Assignee)

Comment 127

16 years ago
over to me to do the merge from branch -> trunk.
Assignee: dveditz → ssu
Status: REOPENED → NEW

Comment 128

16 years ago
Installer triage team: nsbeta1+/adt1
Whiteboard: [adt1 RTM] [ETA 05/25][not fixed on trunk] → [adt1][not fixed on trunk]
(Assignee)

Comment 129

16 years ago
Created attachment 114155 [details] [diff] [review]
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...
(Reporter)

Updated

16 years ago
QA Contact: ruixu → ylong
(Assignee)

Comment 130

16 years ago
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
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 131

16 years ago
oops, instead of bug 125106, I meant bug 192166.

Comment 132

16 years ago
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
(Assignee)

Comment 135

16 years ago
Created attachment 119753 [details] [diff] [review]
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).
Attachment #114155 - Attachment is obsolete: true
(Assignee)

Comment 136

16 years ago
Created attachment 119754 [details]
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
(Assignee)

Updated

16 years ago
Attachment #119753 - Flags: review?(dveditz)
(Assignee)

Comment 137

16 years ago
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

Comment 138

16 years ago
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.
(Assignee)

Comment 139

16 years ago
The uninstall problem is bug 143514 and possible also bug 137656.

Comment 140

16 years ago
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.
(Assignee)

Comment 141

16 years ago
Created attachment 121144 [details] [diff] [review]
patch v1.3

fixed some string conversion problems and also added fix for bug 187979.
Attachment #119753 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #119753 - Flags: review?(dveditz)

Comment 142

16 years ago
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
)
(Assignee)

Comment 143

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

r=ssu
Attachment #121144 - Flags: review+
(Assignee)

Comment 144

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

agh, wrong bug. sorry.
Attachment #121144 - Flags: review+
(Assignee)

Comment 145

15 years ago
Created attachment 121928 [details] [diff] [review]
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.
Attachment #121144 - Attachment is obsolete: true
(Assignee)

Comment 146

15 years ago
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?
(Assignee)

Updated

15 years ago
Attachment #121928 - Flags: superreview?(alecf)
Attachment #121928 - Flags: review?(dveditz)

Comment 147

15 years ago
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-
(Assignee)

Comment 149

15 years ago
Created attachment 122067 [details] [diff] [review]
patch v1.5
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+
(Assignee)

Comment 152

15 years ago
patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 153

15 years ago
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

Updated

15 years ago
Flags: blocking1.4b?
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.