241282 - When choosing an alternative location via "Browse...", the Installer "defaults" to ns_temp subfolder

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Robert Marshall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040421 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040421 Firefox/0.8.0+

When installing either Firefox or Thunderbird, using the Installer build, if you
do not choose the default install location of c:\program files\Mozilla Firefox,
it defaults to the ns_temp subfolder, and you have to navigate to the folder you
want to install to.

Reproducible: Always
Steps to Reproduce:
1. Run installer program
2. When asked if you want to install to default directory, choose Browse
3. browse window opens up defaulting to c:\Documents and Settings\root\Local
Settings\Temp\ns_temp subfolder

Actual Results:  
I had to navigate to the folder location I wanted to install to.

Expected Results:  
It should open at the c:\program files folder, rather than some hidden
subfolder. That makes it a lot easier to find the folder I want.

This happens with the Installer version of both Firefox and Thunderbird, from
4/21/04

Comment 1

[Robert Marshall]

Same issue with 5/2/04 builds.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

-ing will take patch

Comment 3

[Jeff Schiller]

Attachment: Screenshot of bug

Screenshot

Comment 4

[Jon Henry]

Attachment: Proposed patch

This reads the path to the user's Program Files directory from the registry and
uses that, rather than the temp dir. I've tested it successfully on my WinXP
system under both admin and limited user accounts, but I can't guarantee it
works on other setups (particularly Win 98). Feedback would be appreciated.

There's some other stuff which was just me fixing wonky spacing in the
function, but if that's unwanted I can roll up a patch without that.

Comment 5

[Jon Henry]

Comment on attachment 153121 [details] [diff] [review]
Proposed patch

Ben, could you please look at this when you get the chance?

Comment 6

[Bill Mason]

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

Comment 7

[Bill Mason]

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

Comment 8

[Jesse Ruderman]

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

Comment 9

[Stephen Walker]

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

Comment 10

[Radek 'sysKin' Czyz]

Nominating for 1.1 because of its evilness and because a patch exists. Oh, and
because this bug makes it so easy to install to temp folder...

Comment 11

[Frank Wein [:mcsmurf]]

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

Comment 12

[Worcester12345]

(In reply to comment #10)
> Nominating for 1.1 because of its evilness and because a patch exists. Oh, and
> because this bug makes it so easy to install to temp folder...

Can someone add "dataloss" keyword?

Comment 13

[Worcester12345]

Is this patch working or does it need updating?

Comment 14

[Benjamin Smedberg]

Comment on attachment 153121 [details] [diff] [review]
Proposed patch

This should not be using the registry directly. The correct windows APIs are
SHGetSpecialFolderLocation or SHGetSpecialFolderPath, but the *Location version
is available on plain-jane win95 while the *Path version is not: could you
please resubmit this patch using SHGetSpecialFolderLocation?

Comment 15

[Jon Henry]

Attachment: Revised patch

Like this? The patch seems to compile and work for me.

Comment 16

[Benjamin Smedberg]

Comment on attachment 186875 [details] [diff] [review]
Revised patch

SHGetSpecialFolderLocation can fail; please check it for success before calling
SHGetPathFromIDList, and if it fails manually set something useful like
GetCurrentDirectory or "C:\".

Comment 17

[Jon Henry]

Attachment: Third try

Error checking is good. How does this look now, Benjamin?

Comment 18

[Benjamin Smedberg]

Comment on attachment 187051 [details] [diff] [review]
Third try

Calling SHGetPathFromIDList after SHGetSpecialFolderLocation fails doesn't
sound good.

if (GetFolderLocation != S_OK ||
  SHGetPathFromIDList != S_OK)
  strncpy...

Comment 19

[Jon Henry]

Attachment: Next...

You're right...thanks for sticking with this.

Comment 20

[Jon Henry]

Thanks for the review comments and approval. I have no CVS access, could someone
please check this in when you have the chance? Thanks.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Checking in toolkit/mozapps/installer/windows/wizard/setup/dialogs.c;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c,v  <--
 dialogs.c
new revision: 1.34; previous revision: 1.33
done

Comment 22

[Scott MacGregor]

This patch broke my build. I get a compiler error in dialog.c now complaining
about CSIDL_PROGRAM_FILES not being defined anywhere.

c:/build/trees/dbg/mozilla/toolkit/mozapps/installer/windows/wizard/setup/dialog
s.c(620) : error C2065: 'CSIDL_PROGRAM_FILES' : undeclared identifier
make[6]: *** [dialogs.obj] Error 2
make[6]: Leaving directory `/cygdrive/c/build/trees/dbg/mozilla/toolkit/mozapps/
installer/windows/wizard/setup'
make[5]: *** [libs] Error 2

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: fix bustage

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Scott:

What are you using the build environment? MinGW?
# beast and pacifica are successed.

Comment 25

[Scott MacGregor]

patrocles is red due to this checkin so not all of the official build machines
are building.

As for me, I use MS studio 6.0 to build.

This patch fixed the bustage for me.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed-out.

# I will sleep soon, sorry.

Comment 27

[Darin Fisher]

The check-in appears to have also broken the Win32 XULRunner tinderbox, so be
sure to keep an eye on that tinderbox when you land this patch again.

Comment 28

[Benjamin Smedberg]

Comment on attachment 187224 [details] [diff] [review]
fix bustage

Is this constant not in older SDK headers? Does that mean it is not honored in
some versions of Windows? In any case, r+a=me to land this along with the other
patch.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 30

[baffclan]

I tested Firefox trunk build(2005062606)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+

custom install folder in dialogue is
Actual Results:
C:\Program Files

Expected Results:
C:\Program Files\Deer Park Alpha1

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #30)
> I tested Firefox trunk build(2005062606)
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050626
> Firefox/1.0+
> 
> custom install folder in dialogue is
> Actual Results:
> C:\Program Files
> 
> Expected Results:
> C:\Program Files\Deer Park Alpha1


If first install, "C:\Program Files\Deer Park Alpha1" may not be exist.
I think that always using "C:\Program Files" is better solution.

Comment 32

[Kevin Brosnan]

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

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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