bug#13065: Bug in x-file-dialog with GetOpenFileName

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Du Yanning]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

Platform: Windows 7
Emacs version: 24.2.1

Steps to reproduce this bug:
emacs -Q

copy and paste the next line into the *scratch* buffer:
(x-file-dialog "hi" "c:\\")

C-x C-e to evaluate it.
The dialog appears.

Type "abc" (without the enclosing double quotes) in the "File name" field.
Click the "Desktop" icon on the left side of the dialog.
Click the "Open" button.
The dialog does NOT disappear while it should.

I have tried GetOpenFileName/GetSaveFileName in my own Win32 programs and
found that this behavior is casued by GetOpenFileName and GetSaveFileName
is OK in such situation. However, simply replacing GetOpenFileName with
GetSaveFileName will break other code in x-file-dialog.

[-- Attachment #2: Type: text/html, Size: 817 bytes --]

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> Date: Mon, 3 Dec 2012 19:09:49 +0800
> From: Du Yanning <duyanning@gmail.com>
> 
> Platform: Windows 7
> Emacs version: 24.2.1
> 
> Steps to reproduce this bug:
> emacs -Q
> 
> copy and paste the next line into the *scratch* buffer:
> (x-file-dialog "hi" "c:\\")
> 
> C-x C-e to evaluate it.
> The dialog appears.
> 
> Type "abc" (without the enclosing double quotes) in the "File name" field.
> Click the "Desktop" icon on the left side of the dialog.
> Click the "Open" button.
> The dialog does NOT disappear while it should.

This works fine on XP.  On Windows 7, I indeed see the problem, and it
seems to be related to any action in the dialog that changes the
original directory, which results in receiving in the dialog hook
procedure of the CDN_FOLDERCHANGE and CDN_SELCHANGE notifications
(which we don't handle).  If you just type "abc" and click "Open", the
dialog works as expected.  It also works if you do change to another
directory and select an existing file.

> I have tried GetOpenFileName/GetSaveFileName in my own Win32 programs and
> found that this behavior is casued by GetOpenFileName and GetSaveFileName
> is OK in such situation.

GetSaveFileName only works with existing files, which might explain
why it works where GetOpenFileName fails.

I tried to get this to work on Windows 7, or at least figure out why
it doesn't, but came up empty-handed.  Which is not surprising, since
I know absolutely nothing about Windows GUI dialogs in general, and
this dialog in particular.  It doesn't particularly help that:

 a) Windows 7 deprecated this kind of dialogs and instead wants us to
    use some hotshot new ones.  So it could be simply a bug in the
    implementation of this dialog on Windows 7.

 b) We use a file selection dialog in non-standard ways, to be able to
    open directories, not just files.  The way we do this employs some
    undocumented tricks which I don't fully understand.  E.g., when
    the hook gets the CDN_INITDONE notification, we access the File
    Name field of the dialog, but don't do anything with it, and just
    call EnableWindow.  Is that really necessary, and if so, why?

 c) The filter string uses some "*|*" magic in the directory filter,
    which doesn't seem to be documented anywhere.  What does it do?

I CC Jason, who is the author of this code, in the hope that he will
be able to give some advice.  Failing that, I really don't know what
to do to fix this.  Ideas and advice will be most appreciated.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Jason Rumney]

Sorry for the late reply.


Eli Zaretskii <eliz@gnu.org> writes:

>  a) Windows 7 deprecated this kind of dialogs and instead wants us to
>     use some hotshot new ones.  So it could be simply a bug in the
>     implementation of this dialog on Windows 7.

Is it not just a case of doing the same as we did previously?

  /* If we compile with _WIN32_WINNT set to 0x0400 (for NT4
     compatibility) we end up with the old file dialogs. Define a big
     enough struct for the new dialog to trick GetOpenFileName into
     giving us the new dialogs on newer versions of Windows.  */


>  b) We use a file selection dialog in non-standard ways, to be able to
>     open directories, not just files.  The way we do this employs some
>     undocumented tricks which I don't fully understand.  E.g., when
>     the hook gets the CDN_INITDONE notification, we access the File
>     Name field of the dialog, but don't do anything with it, and just
>     call EnableWindow.  Is that really necessary, and if so, why?


The purpose is to disable the File Name field when "Directories" is
selected so the user does not try to type a filename (which will
subsequently be ignored).  "Current Directory" is put into the field
when it is disabled to indicate to the user that the current directory
will be chosen if they press OK.

>
>  c) The filter string uses some "*|*" magic in the directory filter,
>     which doesn't seem to be documented anywhere.  What does it do?

Nothing.  It has to be different than the *.* that is used for normal
file selection, but couldn't be simply * IIRC. The magic is in the
callback function and the code that strips away the filename when the
dialog call has finished if the "Directories" option is selected.

But in this report, the Directory filter was never chosen, so I don't
think it is related.  You could try removing the Directories portion of
the filter string and the callback to be sure.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> From: Jason Rumney <jasonr@gnu.org>
> Cc: Du Yanning <duyanning@gmail.com>,  13065@debbugs.gnu.org
> Date: Tue, 08 Jan 2013 21:10:35 +0800
> 
> 
> Sorry for the late reply.

Thanks for chiming in.

> >  a) Windows 7 deprecated this kind of dialogs and instead wants us to
> >     use some hotshot new ones.  So it could be simply a bug in the
> >     implementation of this dialog on Windows 7.
> 
> Is it not just a case of doing the same as we did previously?
> 
>   /* If we compile with _WIN32_WINNT set to 0x0400 (for NT4
>      compatibility) we end up with the old file dialogs. Define a big
>      enough struct for the new dialog to trick GetOpenFileName into
>      giving us the new dialogs on newer versions of Windows.  */

Sorry, I don't understand: we still do what the above comment
describes.

Thanks for the other info.  Unfortunately, I'm still clueless as to
what could be causing the problem.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Jason Rumney]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is it not just a case of doing the same as we did previously?
>> 
>>   /* If we compile with _WIN32_WINNT set to 0x0400 (for NT4
>>      compatibility) we end up with the old file dialogs. Define a big
>>      enough struct for the new dialog to trick GetOpenFileName into
>>      giving us the new dialogs on newer versions of Windows.  */
>
> Sorry, I don't understand: we still do what the above comment
> describes.

I meant maybe we need to increase the size of the struct again to fool
Windows 7 into thinking we are compiling against a more recent version
of the SDK.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> From: Jason Rumney <jasonr@gnu.org>
> Cc: duyanning@gmail.com,  13065@debbugs.gnu.org
> Date: Sun, 13 Jan 2013 20:52:25 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is it not just a case of doing the same as we did previously?
> >> 
> >>   /* If we compile with _WIN32_WINNT set to 0x0400 (for NT4
> >>      compatibility) we end up with the old file dialogs. Define a big
> >>      enough struct for the new dialog to trick GetOpenFileName into
> >>      giving us the new dialogs on newer versions of Windows.  */
> >
> > Sorry, I don't understand: we still do what the above comment
> > describes.
> 
> I meant maybe we need to increase the size of the struct again to fool
> Windows 7 into thinking we are compiling against a more recent version
> of the SDK.

Thanks, will try that some time soon.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> Date: Sun, 13 Jan 2013 18:35:41 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 13065@debbugs.gnu.org, duyanning@gmail.com
> 
> > From: Jason Rumney <jasonr@gnu.org>
> > Cc: duyanning@gmail.com,  13065@debbugs.gnu.org
> > Date: Sun, 13 Jan 2013 20:52:25 +0800
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> Is it not just a case of doing the same as we did previously?
> > >> 
> > >>   /* If we compile with _WIN32_WINNT set to 0x0400 (for NT4
> > >>      compatibility) we end up with the old file dialogs. Define a big
> > >>      enough struct for the new dialog to trick GetOpenFileName into
> > >>      giving us the new dialogs on newer versions of Windows.  */
> > >
> > > Sorry, I don't understand: we still do what the above comment
> > > describes.
> > 
> > I meant maybe we need to increase the size of the struct again to fool
> > Windows 7 into thinking we are compiling against a more recent version
> > of the SDK.
> 
> Thanks, will try that some time soon.

I spent some more time tinkering with this.

First, the new Windows 7 dialogs are not merely an extension of the
old ones, they are an entirely different implementation, via COM
objects and a C++ interface.  They also don't accept a structure
similar to OPENFILENAME.  So the trick to expand the structure is not
going to help us.

> >  c) The filter string uses some "*|*" magic in the directory filter,
> >     which doesn't seem to be documented anywhere.  What does it do?
> 
> Nothing.  It has to be different than the *.* that is used for normal
> file selection, but couldn't be simply * IIRC. The magic is in the
> callback function and the code that strips away the filename when the
> dialog call has finished if the "Directories" option is selected.
> 
> But in this report, the Directory filter was never chosen, so I don't
> think it is related.  You could try removing the Directories portion of
> the filter string and the callback to be sure.

Tried without the Directories portion, didn't help.

I tried to attack this from a different angle.  I compiled a simple
stand-alone program (source below) that just opens the file selector
dialog and prints the selected name after you click Open.  This
program works fine both on XP and on Windows 7, and shows a dialog
that is almost exactly the same as what Emacs displays -- the only
difference, which only shows on Windows 7, is in the slightly
different visual appearance of the "Open" and "Cancel" buttons (on XP
the dialog is identical to what Emacs shows, including the buttons).

(To show dialogs like Emacs does, compile the program with the
"-D_WIN32_WINNT=0x0501" switch, otherwise you get old-style dialogs.)

Then I examined the Emacs sources for any differences between how we
set up the dialog and this sample program, and modified the Emacs
source to exactly match the sample program, right down to use of
ZeroMemory instead of memset (ridiculous, I know; but I was
desperate).  Nothing helped: the same code in Emacs still won't close
the dialog and return the file name, unless I select an existing
file.  By contrast, the sample program below will return any file
name, whether or not that file exists, in any directory.

Maybe I'm blind and missed some difference that I didn't eliminate.
You are welcome to try that experiment and see if you succeed where I
failed.

Failing that, I'm beginning to think that the problem is elsewhere in
Emacs.  But what could that be?  Is it possible that one of our
message pumps somehow interferes with the dialog interaction, e.g., by
intercepting some of the messages that were supposed to wind up in our
hook procedure?  Could it be due to the fact that our main message
pump uses Unicode APIs, while the file selector uses ANSI APIs?
Something else?

I'm stumped.

Here's the program to try:

#include <stdio.h>
#include <windows.h>

static UINT_PTR CALLBACK
file_dialog_callback (HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
{
  return 0;
}

int
main (void)
{
  OPENFILENAMEA ofn;       // common dialog box structure
  char szFile[260];       // buffer for file name

  // Initialize OPENFILENAME
  ZeroMemory(&ofn, sizeof(ofn));
  ofn.lStructSize = sizeof(ofn);
  ofn.hwndOwner = NULL;
  ofn.lpstrFile = szFile;
  // Set lpstrFile[0] to '\0' so that GetOpenFileName does not 
  // use the contents of szFile to initialize itself.
  ofn.lpstrFile[0] = '\0';
  ofn.nMaxFile = sizeof(szFile);
  ofn.lpstrFilter = "All\0*.*\0Text\0*.TXT\0";
  ofn.nFilterIndex = 1;
  ofn.lpstrFileTitle = NULL;
  ofn.nMaxFileTitle = 0;
  ofn.lpstrInitialDir = NULL;
  ofn.Flags = (OFN_HIDEREADONLY | OFN_NOCHANGEDIR | OFN_EXPLORER | OFN_ENABLEHOOK);
  ofn.lpfnHook = file_dialog_callback;

  // Display the Open dialog box.

  if (GetOpenFileNameA(&ofn)==TRUE)
    printf ("GetOpenFile: '%s'\n", ofn.lpstrFile);
  else
    printf ("Failure: 0x%x\n", (unsigned)CommDlgExtendedError ());

  return 0;
}

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Jason Rumney]

Eli Zaretskii <eliz@gnu.org> writes:

> Failing that, I'm beginning to think that the problem is elsewhere in
> Emacs.  But what could that be?  Is it possible that one of our
> message pumps somehow interferes with the dialog interaction, e.g., by
> intercepting some of the messages that were supposed to wind up in our
> hook procedure?

That seems like the most likely candidate. Do you have Spy++, or an
equivalent that can show the Windows messages going to and from your
sample program and the Open/Save dialog in Emacs?

> Could it be due to the fact that our main message
> pump uses Unicode APIs, while the file selector uses ANSI APIs?

Since that's quite a recent change, it should be easy to test by trying
an older version of Emacs on Windows 7.

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> From: Jason Rumney <jasonr@gnu.org>
> Cc: 13065@debbugs.gnu.org,  duyanning@gmail.com
> Date: Wed, 16 Jan 2013 20:23:19 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Failing that, I'm beginning to think that the problem is elsewhere in
> > Emacs.  But what could that be?  Is it possible that one of our
> > message pumps somehow interferes with the dialog interaction, e.g., by
> > intercepting some of the messages that were supposed to wind up in our
> > hook procedure?
> 
> That seems like the most likely candidate. Do you have Spy++, or an
> equivalent that can show the Windows messages going to and from your
> sample program and the Open/Save dialog in Emacs?
> 
> > Could it be due to the fact that our main message
> > pump uses Unicode APIs, while the file selector uses ANSI APIs?
> 
> Since that's quite a recent change, it should be easy to test by trying
> an older version of Emacs on Windows 7.

Yes, I also had the idea to check older versions.  It was a good idea,
because it allowed me to find the reason for the problem.  It wasn't
an interference between message pumps.  It was memory.  Here are the
details.

In preparation for Emacs 24.1, we changed the amount of memory we
reserve at startup: where Emacs 23 asked the OS to reserve 256MB, we
now try to reserve as much as possible, starting with 2GB and
gradually decreasing the amount until the OS grants the request.  On
XP, we typically end up with 1.1 to 1.5GB, but on Windows 7 we get
more: about 1.8GB.

Now, when we launch the file selector dialog, Windows starts several
threads in our process to support that (I see 4 threads on XP and
between 6 and 7 on Windows 7).  My guess is that with so much of the
address space reserved, these threads fail to reserve enough memory
for themselves, probably for their stack (which is set to 8MB in
Emacs), and misbehave as result of that.  We already had a similar
problem with the threads we ourselves create to support subprocesses,
see
http://lists.gnu.org/archive/html/emacs-devel/2012-03/msg00119.html.
In that case, we were able to fix the problem without decreasing the
amount of memory we ask at startup.  But here, these are threads that
are started outside our control, so a similar solution is not
possible, AFAIU.

So, unless someone has a better idea, I will soon commit to the
emacs-24 branch the patch shown below.  It fixes the problem on
Windows 7 and does not affect XP in any visible way.  The initial
amount of memory to reserve was found by trial and error.

(After this patch, the original recipe still doesn't work: you need to
type "abc" _after_ clicking "Desktop".  But the same happens with
Emacs 23, so I guess this is just a minor change in behavior of
Windows 7.)

=== modified file 'src/w32heap.c'
--- src/w32heap.c	2013-01-01 09:11:05 +0000
+++ src/w32heap.c	2013-01-16 18:34:38 +0000
@@ -98,7 +98,11 @@ allocate_heap (void)
 #ifdef _WIN64
   size_t size = 0x4000000000i64; /* start by asking for 32GB */
 #else
-  size_t size = 0x80000000; /* start by asking for 2GB */
+  /* We used to start with 2GB here, but on Windows 7 that would leave
+     too little room in the address space for threads started by
+     Windows on our behalf, e.g. when we pop up the file selection
+     dialog.  */
+  size_t size = 0x68000000; /* start by asking for 1.7GB */
 #endif
   void *ptr = NULL;
 

bug#13065: Bug in x-file-dialog with GetOpenFileName

[Eli Zaretskii]

> Date: Wed, 16 Jan 2013 20:54:05 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 13065@debbugs.gnu.org, duyanning@gmail.com
> 
> So, unless someone has a better idea, I will soon commit to the
> emacs-24 branch the patch shown below.  It fixes the problem on
> Windows 7 and does not affect XP in any visible way.  The initial
> amount of memory to reserve was found by trial and error.
> 
> (After this patch, the original recipe still doesn't work: you need to
> type "abc" _after_ clicking "Desktop".  But the same happens with
> Emacs 23, so I guess this is just a minor change in behavior of
> Windows 7.)
> 
> === modified file 'src/w32heap.c'
> --- src/w32heap.c	2013-01-01 09:11:05 +0000
> +++ src/w32heap.c	2013-01-16 18:34:38 +0000
> @@ -98,7 +98,11 @@ allocate_heap (void)
>  #ifdef _WIN64
>    size_t size = 0x4000000000i64; /* start by asking for 32GB */
>  #else
> -  size_t size = 0x80000000; /* start by asking for 2GB */
> +  /* We used to start with 2GB here, but on Windows 7 that would leave
> +     too little room in the address space for threads started by
> +     Windows on our behalf, e.g. when we pop up the file selection
> +     dialog.  */
> +  size_t size = 0x68000000; /* start by asking for 1.7GB */
>  #endif
>    void *ptr = NULL;

No further comments, so I committed this to the emacs-24 branch
(revision 111192), and I'm closing this bug report.