282271 - "Show File Location" does not work on Windows Terminal Services

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Ian Neal]

Using BuildID 2005013106 on Win2K SP3
Steps to reproduce
1. Map a netware drive (to T drive for example)
2. Download a file to that mapped drive
3. Click on "Show File Location" in download manager

Expected Results
1. Windows Explorer window opens at correct location

Actual Results
1. No Windows Explorer window opens
2. Following error message in JS console:
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsILocalFile.reveal]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://communicator/content/downloadmanager/downloadmanager.js ::
dVC_doCommand :: line 273"  data: no]
Source File: chrome://communicator/content/downloadmanager/downloadmanager.js
Line: 273

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sounds like a bug in nsLocalFileWin (presumably ::ShellExecute returns something
< 32 ?  See http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#2211

Comment 2

[Doug Turner (:dougt)]

I have no idea how to correctly open a network path via explorer.exe and shell
execute.  anyone know off hand?

Comment 3

[James Ross]

Both the following (assuming M is mapped to \\server\share) work fine here on 2K:

  explorer /n,/select,"\\server\share\path\file.ext"
  explorer /n,/select,"M:\path\file.ext"

However, that appears to be what the code is doing (except it uses full paths to
explorer - which also works fine here).

Comment 4

[Ian Neal]

I've tried the explorer command on the Win2K box and that works fine from the
run command menu box within the start menu but still fails from download manager
with the error message stated before.

Comment 5

[Ian Neal]

If someone can tell me the correct command for capturing what is causing this
problem via gdb and what output you want from it then I will do it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, the question is why nsILocalFile.launch() fails.  As far as I can see,
that devolves to the question of why ::ShellExecute fails...  See link in comment 1.

Comment 7

[Ian Neal]

I replaced lines 2211-12 with:

    LONG r = (LONG) ::ShellExecute(NULL, "open", explorerPath.get(),
explorerParams.get(), NULL, SW_SHOWNORMAL);
    if (r < 32) {
       switch (r) {
         case 0:
         case SE_ERR_OOM:
           return NS_ERROR_OUT_OF_MEMORY;
         case ERROR_FILE_NOT_FOUND:
           return NS_ERROR_FILE_NOT_FOUND;
         case ERROR_PATH_NOT_FOUND:
           return NS_ERROR_FILE_UNRECOGNIZED_PATH;
         case ERROR_BAD_FORMAT:
           return NS_ERROR_FILE_CORRUPTED;
         case SE_ERR_ACCESSDENIED:
           return NS_ERROR_FILE_ACCESS_DENIED;
         case SE_ERR_ASSOCINCOMPLETE:
         case SE_ERR_NOASSOC:
           return NS_ERROR_UNEXPECTED;
         case SE_ERR_DDEBUSY:
         case SE_ERR_DDEFAIL:
         case SE_ERR_DDETIMEOUT:
           return NS_ERROR_NOT_AVAILABLE;
         case SE_ERR_DLLNOTFOUND:
           return NS_ERROR_FAILURE;
         case SE_ERR_SHARE:
           return NS_ERROR_FILE_IS_LOCKED;
         default:
           return NS_ERROR_FILE_EXECUTION_FAILED;
       }
   }

and the message I now get in the JS console is:
Error: [Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsILocalFile.reveal]"  nsresult: "0x80520012
(NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
chrome://communicator/content/downloadmanager/downloadmanager.js ::
dVC_doCommand :: line 273"  data: no]
Source File: chrome://communicator/content/downloadmanager/downloadmanager.js
Line: 273

The launch file option does work though.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #7)
>     LONG r = (LONG) ::ShellExecute(NULL, "open", explorerPath.get(),
> explorerParams.get(), NULL, SW_SHOWNORMAL);

what are explorerPath.get() and explorerParams.get() here?

Comment 9

[timeless]

if you're using \\server\share\patch\file, try \\?\\server\share\patch\file (i
think that's right, it's been a while).

Comment 10

[Ian Neal]

This is running on Windows 2000 Server SP3 with Citrix Metaframe XPe FR3
Output from printf of those variables is:
explorerPath: C:\Documents and
Settings\administrator.SPRINGFIELD\WINDOWS\explorer.exe
explorerParams: /n,/select,"T:\Download\Sun\Solaris8\top-3.5.1-sol8-sparc-local.gz"

Set at a command prompt gives:
SystemRoot=C:\WINNT 
windir=C:\WINNT

I cannot actually find anything in the registry saying:
C:\Documents and Settings\administrator.SPRINGFIELD\WINDOWS

Comment 11

[Boris Zbarsky [:bzbarsky]]

So... this code is using Win_WindowsDirectory, which is implemented via
::GetWindowsDirectory (see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getwindowsdirectory.asp).

Should it be using Win_SystemDirectory instead?  That uses ::GetSystemDirectory
(see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getsystemdirectory.asp)?

Neither one really quite fits the description of "the place I'd look for IE"
based on the MSDN descriptions, but...

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #10)
> explorerPath: C:\Documents and
> Settings\administrator.SPRINGFIELD\WINDOWS\explorer.exe
> explorerParams:
/n,/select,"T:\Download\Sun\Solaris8\top-3.5.1-sol8-sparc-local.gz"

Does explorer.exe exist at that location?

(In reply to comment #11)
> Neither one really quite fits the description of "the place I'd look for IE"
> based on the MSDN descriptions, but...

Hm? Why IE?

Comment 13

[Boris Zbarsky [:bzbarsky]]

s/IE/explorer/...

Comment 14

[Ian Neal]

(In reply to comment #12)
> (In reply to comment #10)
> > explorerPath: C:\Documents and
> > Settings\administrator.SPRINGFIELD\WINDOWS\explorer.exe
> > explorerParams:
> /n,/select,"T:\Download\Sun\Solaris8\top-3.5.1-sol8-sparc-local.gz"
> 
> Does explorer.exe exist at that location?
No explorer.exe sits in c:\winnt

Quoting from
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getwindowsdirectory.asp

" The Windows directory is the directory where an application should store
initialization and help files. If the user is running a shared version of the
system, the Windows directory is guaranteed to be private for each user.

If an application creates other files that it wants to store on a per-user
basis, it should place them in the directory specified by the HOMEPATH
environment variable. This directory will be different for each user, if so
specified by an administrator, through the User Manager administrative tool.
HOMEPATH always specifies either the user's home directory, which is guaranteed
to be private for each user, or a default directory (for example,
C:\USERS\DEFAULT) where the user will have all access.

    Terminal Services:   If the application is running in a Terminal Services
environment, each user has a private Windows directory. There is also a shared
Windows directory for the system. If the application is Terminal-Services-aware
(has the IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE flag set in the image
header), this function returns the path of the system Windows directory, just as
the GetSystemWindowsDirectory function does. Otherwise, it retrieves the path of
the private Windows directory for the user. "

So it looks like we need to set the
IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE flag in the image header
otherwise we get the wrong path.

Comment 15

[Ian Neal]

Old summary - "Show File Location" does not work on network drives

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er... setting IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE will break all the
callers that _do_ want the user directory.  Of which there ought to be a few.

Sounds like this code should be using a different directory service constant;
whichever one corresponds to GetSystemWindowsDirectory.

Comment 17

[Ian Neal]

Reading through MSDN library looks we need to do the following
For Win9x we need to keep using GetWindowsDirectory
For WinNT we need to use GetSystemDirectory and trim the "System32" element from
the end of the returned path
For Win2k/XP we need to use GetSystemWindowsDirectory

Urgh!

Comment 18

[Boris Zbarsky [:bzbarsky]]

So.. see the list at
http://lxr.mozilla.org/seamonkey/source/xpcom/io/SpecialSystemDirectory.h#83

If none of those fit the description, sounds like we need a new constant, an
impl (that checks the OS version), and an audit of Win_* directory users to see
what they should be using.

Comment 19

[Boris Zbarsky [:bzbarsky]]

In particular, there's the question of what the (frozen) nsIDirectoryService
should be doing for the "WinD" string.

Comment 20

[Ian Neal]

I've tried adding this:
        case Win_SystemWindowsDirectory:
        {
            // GetSystemWindowsDirectory is only supported on Win2K upwards,
            // so for other Windows versions need to provide correct path
            // as specified in MSDN library
            PRInt32 len;
            OSVERSIONINFO info = { sizeof(OSVERSIONINFO) };
            if (GetVersionEx(&info)) {
                if ( info.dwPlatformId == VER_PLATFORM_WIN32_NT ) {
                    if (info.dwMajorVersion == 4) {
                        len = GetSystemDirectory( path, _MAX_PATH );
                        // Trim System32 element from path
                        len -= 9;
                    } else if (info.dwMajorVersion == 5)
                        len = GetSystemWindowsDirectory( path, _MAX_PATH );
                    else
                        len = GetWindowsDirectory( path, _MAX_PATH );
                } else
                    len = GetWindowsDirectory( path, _MAX_PATH );
            } else
                len = GetWindowsDirectory( path, _MAX_PATH );

            // Need enough space to add the trailing backslash
            if (len > _MAX_PATH-2)
                break;
            
            path[len]   = '\\';
            path[len+1] = '\0';

            return NS_NewNativeLocalFile(nsDependentCString(path), 
                                         PR_TRUE, 
                                         aFile);
        }

but it won't compile on cygwin, I get the following error messages and it fails:
c:/mozdev/mozilla/xpcom/io/SpecialSystemDirectory.cpp: In function `nsresult 
   GetSpecialSystemDirectory(SystemDirectories, nsILocalFile**)':
c:/mozdev/mozilla/xpcom/io/SpecialSystemDirectory.cpp:331: warning: unused 
   variable `DWORD len'
c:/mozdev/mozilla/xpcom/io/SpecialSystemDirectory.cpp:477: `
   GetSystemWindowsDirectory' undeclared (first use this function)
c:/mozdev/mozilla/xpcom/io/SpecialSystemDirectory.cpp:477: (Each undeclared 
   identifier is reported only once for each function it appears in.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

And this file includes windows.h?

Comment 22

[Ian Neal]

Attachment: Working patch v0.1

This patch makes use of GetProcAddress, as advised by Neil on IRC, so that it
should still compile on Win9x.
I have tested on normal Windows XP and Windows 2000 Server with Terminal
Services.

Comment 23

[Ian Neal]

The other user of Win_WindowsDirectory is NS_WIN_WINDOWS_DIR via
nsDirectoryService::sWindowsDirectory
http://lxr.mozilla.org/seamonkey/ident?i=NS_WIN_WINDOWS_DIR

I do not know enough about the bits of code that make use of NS_WIN_WINDOWS_DIR
to determine if they need Win_WindowsDirectory or Win_SystemWindowsDirectory.
For example the nsProfile.cpp I think needs to grab nsreg.dat but on Windows
2000 Server with Terminal Services there is an nsreg.dat in C:\WINNT and
C:\Documents and Settings\<username>\WINDOWS so does it need to grab both or
just one of them?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'd file followup bugs on those uses, one bug per affected module....

Comment 25

[Ian Neal]

Attachment: Revised patch v0.1a

Changes in this patch as per timeless' suggestions on #developers
* Put new code in Win_WindowsDirectory can renamed old code with new number
(228)
* Changed nsDirectoryService to use old code's new name
Win_UserWindowsDirectory
* Added {} round the code after the else in the new code

Also added missing Win_Cookies to xpcom_consts.py

Comment 26

[timeless]

Comment on attachment 176778 [details] [diff] [review]
Revised patch v0.1a

i gave further comments (handle nt3,nt6+), and the need to add another atom. we
should probably have some js xpcshell testcases for this stuff.

Comment 27

[Ian Neal]

Attachment: Atomised patch v0.1b

Changes since last patch
* pre-NT4 and NT6+ catered for
* new Atom created

Any pointers on how to create these js xpcshell testcases?

Comment 28

[Ian Neal]

Attachment: Unbitrotted patch v0.1c

Comment 29

[Ian Neal]

adding helpwanted keyword as I need help putting together the xpcshell testcases

Comment 30

[timeless]

sorry about the delay, try the attachments in https://bugzilla.mozilla.org/buglist.cgi?bug_id=209110,225055,241200,241708,241710,242110,277367 

for examples of testcases. it seems none of my fun directory service testcases turned up in my search, but the idea is pretty simple...

Comment 31

[Benjamin Smedberg]

AIUI, this bug only affects pre-winxp, which is our minimum system nowadays.

Comment 32

[[e-mail]]

(In reply to Ian Neal from comment #14)
> So it looks like we need to set the
> IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE flag in the image header
> otherwise we get the wrong path.

(In reply to Boris Zbarsky from comment #16)
> Er... setting IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE will break all
> the callers that _do_ want the user directory.  Of which there ought to be
> a few.

The default value of that flag changed after Visual Studio 6, and nobody complained.