Problems unveiled by the last version.c patch

Hi,

just started with a fresh wine install from CVS. I've tried to install
vc6redistsetup_enu.exe and was not able to select a directory via the
browse dialog.

Some regression testing and debugging showed:

before patch http://cvs.winehq.org/patch.py?id=14894 :

trace:shell:BrsFolderDlgProc hwnd=0x1002e msg=0082 0x00000000 0x00000000
trace:shell:SHGetPathFromIDListAW (pidl=0x63f4f5b8,0x100b720)
trace:ver:RtlGetVersion <-- Windows 98 (L" A ")
trace:shell:SHGetPathFromIDListA (pidl=0x63f4f5b8,0x100b720)
-------- pidl=0x63f4f5b8
[0x63f4f5b8] size=0020 type=1f attr=0x00000000
name="{20d04fe0-3aea-1069-a2d8-08002b30309d}" ((null),(null))
[0x63f4f5cc] size=0025 type=23 attr=0x00000000 name="F:" ("F:\",(null))
[0x63f4f5e5] size=0020 type=31 attr=0x00000010 name="temp" ("temp","")
trace:pidl:_ILIsValue (0x63f4f5b8)
trace:pidl:_ILIsFolder (0x63f4f5b8)
trace:pidl:_ILGetGUIDPointer 0x63f4f5b8
trace:pidl:_ILGetGUIDPointer pdata->type 0x001f
trace:pidl:_ILIsMyComputer (0x63f4f5b8)
trace:pidl:ILGetNext 0x63f4f5b8
trace:pidl:ILGetNext -- 0x63f4f5cc
trace:pidl:_ILIsSpecialFolder (0x63f4f5cc)
trace:pidl:ILGetNext 0x63f4f5cc
trace:pidl:ILGetNext -- 0x63f4f5e5
trace:shell:PathAddBackslashA ("F:\")
trace:pidl:_ILIsSpecialFolder (0x63f4f5e5)
trace:pidl:ILGetNext 0x63f4f5e5
trace:pidl:ILGetNext -- 0x63f4f5f9
trace:shell:SHELL_GetPathFromIDListA -- F:\temp, 0x00000000

after the patch :

trace:shell:BrsFolderDlgProc hwnd=0x1002e msg=0082 0x00000000 0x00000000
trace:shell:SHGetPathFromIDListAW (pidl=0x77e2dbe8,0x100b720)
trace:ver:RtlGetVersion <-- Windows 2000 (L"Service Pack 4")
trace:shell:SHGetPathFromIDListW (pidl=0x77e2dbe8,0x3f0472b4)
-------- pidl=0x77e2dbe8
[0x77e2dbe8] size=0020 type=1f attr=0x00000000
name="{20d04fe0-3aea-1069-a2d8-08002b30309d}" ((null),(null))
[0x77e2dbfc] size=0025 type=23 attr=0x00000000 name="F:" ("F:\",(null))
[0x77e2dc15] size=0020 type=31 attr=0x00000010 name="temp" ("temp","")
trace:pidl:_ILIsValue (0x77e2dbe8)
trace:pidl:_ILIsFolder (0x77e2dbe8)
trace:pidl:_ILGetGUIDPointer 0x77e2dbe8
trace:pidl:_ILGetGUIDPointer pdata->type 0x001f
trace:pidl:_ILIsMyComputer (0x77e2dbe8)
trace:pidl:ILGetNext 0x77e2dbe8
trace:pidl:ILGetNext -- 0x77e2dbfc
trace:pidl:_ILIsSpecialFolder (0x77e2dbfc)
trace:pidl:ILGetNext 0x77e2dbfc
trace:pidl:ILGetNext -- 0x77e2dc15
trace:shell:PathAddBackslashW (L"F:\")
trace:pidl:_ILIsSpecialFolder (0x77e2dc15)
trace:pidl:ILGetNext 0x77e2dc15
trace:pidl:ILGetNext -- 0x77e2dc29
trace:shell:SHELL_GetPathFromIDListW -- L"F:\temp", 0x00000000
trace:shell:SHGetPathFromIDListW -- L"F:\temp", 0x00000000

The main difference (apart from the A/W) is that SHGetPathFromIDListW
returns an extra backslash compared to the A-counterpart.

Any idea where to look next ?

Cheers,

Paul.

[Ivan Leo Puoti]

> The main difference (apart from the A/W) is that SHGetPathFromIDListW
> returns an extra backslash compared to the A-counterpart.
>
> Any idea where to look next ?

It appears that patch 14894 has uncovered a bug in SHGetPathFromIDListW, 
that's probably where you would want to look next.

Ivan.

[Dmitry Timoshkov]

 wrote:
> trace:shell:SHELL_GetPathFromIDListA -- F:\temp, 0x00000000

...
> trace:shell:SHGetPathFromIDListW -- L"F:\temp", 0x00000000
>
> The main difference (apart from the A/W) is that SHGetPathFromIDListW
> returns an extra backslash compared to the A-counterpart.

If you mean a double backslash in the 2nd line of the above quote
then it's just the escaping which debugtsr_w() performs.

[Paul Vriens]

Hi Dmitry,
>  wrote:
> > trace:shell:SHELL_GetPathFromIDListA -- F:\temp, 0x00000000
>
> ...
> > trace:shell:SHGetPathFromIDListW -- L"F:\temp", 0x00000000
> >
> > The main difference (apart from the A/W) is that SHGetPathFromIDListW
> > returns an extra backslash compared to the A-counterpart.
>
> If you mean a double backslash in the 2nd line of the above quote
> then it's just the escaping which debugtsr_w() performs.
>
> --
> Dmitry.

I think the problem lies in the fact the some of the procedures use
Unicode depending on the version and some don't care (or use some
default). Going to the traces I can see:

Call shell32.SHBrowseForFolder(3de5f760) ret=01003c8d
... (always calls SHBrowseForFolderA)
trace:shell:SHBrowseForFolderA
Ret  shell32.SHBrowseForFolder() retval=77e30fc0 ret=01003c8d
...
Call shell32.SHGetPathFromIDList(77e30fc0,0100b720) ret=01003c99
trace:shell:SHGetPathFromIDListAW
... (checking the version)
trace:shell:SHGetPathFromIDListW
...
Ret  shell32.SHGetPathFromIDList() retval=00000001 ret=01003c99
...
Call kernel32.lstrcpyA(0100a360,0100b720 "F") ret=01003ca9

So you see, we are mixing A and W now:
- SHBrowseForFolderA
- SHGetPathFromIDListW
- lstrcpyA

In this case does it mean we (I ?) have to implement SHBrowseForFolderAW
or is that the wrong path (as many calls don't do this and all of them
have to be checked/corrected)?

Cheers,

Paul.

[Rein Klazes]

On Fri, 17 Dec 2004 12:04:46 +0100 (CET), you wrote:
> Hi Dmitry,
> >  wrote:
> > > trace:shell:SHELL_GetPathFromIDListA -- F:\temp, 0x00000000
> >
> > ...
> > > trace:shell:SHGetPathFromIDListW -- L"F:\temp", 0x00000000
> > >
> > > The main difference (apart from the A/W) is that SHGetPathFromIDListW
> > > returns an extra backslash compared to the A-counterpart.
> >
> > If you mean a double backslash in the 2nd line of the above quote
> > then it's just the escaping which debugtsr_w() performs.
> >
> > --
> > Dmitry.
>
> I think the problem lies in the fact the some of the procedures use
> Unicode depending on the version and some don't care (or use some
> default). Going to the traces I can see:
>
> Call shell32.SHBrowseForFolder(3de5f760) ret=01003c8d
> ... (always calls SHBrowseForFolderA)
> trace:shell:SHBrowseForFolderA
> Ret  shell32.SHBrowseForFolder() retval=77e30fc0 ret=01003c8d
> ...
> Call shell32.SHGetPathFromIDList(77e30fc0,0100b720) ret=01003c99
> trace:shell:SHGetPathFromIDListAW
> ... (checking the version)
> trace:shell:SHGetPathFromIDListW
> ...
> Ret  shell32.SHGetPathFromIDList() retval=00000001 ret=01003c99
> ...
> Call kernel32.lstrcpyA(0100a360,0100b720 "F") ret=01003ca9
>
> So you see, we are mixing A and W now:
> - SHBrowseForFolderA
> - SHGetPathFromIDListW
> - lstrcpyA
>
> In this case does it mean we (I ?) have to implement SHBrowseForFolderAW
> or is that the wrong path (as many calls don't do this and all of them
> have to be checked/corrected)?

SHGetPathFromIDList seems to be the wrong one here (looking at native
shell32.dll from Win2K and Win98), it should be synonymous for
SHGetPathFromIDListA.

Can you try attached patch?

Rein.

[Paul Vriens]

Hi Rein,
> SHGetPathFromIDList seems to be the wrong one here (looking at native
> shell32.dll from Win2K and Win98), it should be synonymous for
> SHGetPathFromIDListA.
>
> Can you try attached patch?
>
> Rein.

This fixes my problem. I've cross-checked and indeed SHGetPathFromIDList
should be synonymous for SHGetPathFromIDListA.

Can we expect to have more 'failures' due to the fact that the new
version.c returns more OS's then before? I'm for one seeing a lot more
messages:

fixme:ver:VERSION_GetLinkedDllVersion Unknown EXE OS version 1.0, please
report !!

Thanks,

Paul.

[Rein Klazes]

On Fri, 17 Dec 2004 14:51:15 +0100 (CET), you wrote:
> Hi Rein,
> > SHGetPathFromIDList seems to be the wrong one here (looking at native
> > shell32.dll from Win2K and Win98), it should be synonymous for
> > SHGetPathFromIDListA.
> >
> > Can you try attached patch?
> >
> > Rein.
>
> This fixes my problem. I've cross-checked and indeed SHGetPathFromIDList
> should be synonymous for SHGetPathFromIDListA.

OK, I will send it to wine-patches.
> Can we expect to have more 'failures' due to the fact that the new
> version.c returns more OS's then before?

You could and can still set the OS version in your config file (globally
and per application). But since the config file is not automatically
created anymore, there will be quite some users testing another OS
version soon.

It is nice that you have uncovered a bug that required to revert to
'win98', which some applications do not like.
> I'm for one seeing a lot more
> messages:
>
> fixme:ver:VERSION_GetLinkedDllVersion Unknown EXE OS version 1.0, please
> report !!

Hmm, it does not seem a good idea to set the OS version to Windows 1.0.

Rein.