DestroyMenu: Check if the menu is still used (fix for bug 1486)

[Andreas Mohr]

Hi,

On Mon, Sep 06, 2004 at 11:58:40AM +0200, Michael Kaufmann wrote:
> Hi Dmitry
> 
> >There is a test case in Wine (dlls/user/tests/win.c,test_SetMenu) which 
> >shows
> >that your patch is wrong.
> > 
> >
> 
> This test case also fails on Win9x, as you can see in the code of the 
> test case:
> 
> ok(DestroyMenu(hMenu), "DestroyMenu error %ld\n", GetLastError());
> ok(!IsMenu(hMenu), "menu handle should be not valid after DestroyMenu\n");
> ret = GetMenu(parent);
> /* This test fails on Win9x */
> if (!is_win9x)
>    ok(ret == hMenu, "unexpected menu id %p\n", ret);
> 
> 
> It doesn't make sense to me to check that the menu handle is invalid AND 
> it is still assigned to the window. Windows 2000 only passes this test 
> because it keeps using the menu. It destroys the menu when it's not 
> needed anymore. WINE is not so clever (yet), therefore we have to do it 
> the same way as Windows 9x.
> 
> I think we should modify this testcase. After all, the behavior of 
> DestroyMenu in this situation is undocumented.
> 
> I've searched for other patches on this subject. I was very surprised to 
> see that Andreas Mohr submitted a patch for the same problem about two 
> years ago:
Oh, me? :-)

True, I've been bitten by that issue at that time (and several applications
failed on it, e.g. FilZip 2.01(?)), so I attempted to fix it.

> http://www.winehq.com/hypermail/wine-patches/2002/08/0124.html
> http://www.winehq.com/hypermail/wine-patches/2002/09/0039.html
> 
> He describes the problem very well. It seems that his patch was not 
> checked in, or it has been modified since.
cvs log should help here.
I don't quite remember what happened, but I think we agreed that
my patches caused some regressions with other apps, so it wasn't applied
or so.
Searching wine-devel around that time might help.

> My patch doesn't have the problem that he describes in the second post.
Hmm, the hMenu 0 problems?
Why not?

> >It was said many times already that the real fix for that bug is to move
> >all user object management into wineserver and use the same handle 
> >allocation
> >scheme as the one used for windows.
> > 
> >
> 
> Sorry, I'm new to WINE development and don't know this discussion. Maybe 
> you can provide a link?
You probably need to allocate menu objects in the server, and since the
server keeps a refcount for every object, it would know when to destroy
the menu and when not to destroy it.
This should be independent from any handle (de-)registration (in hWnds),
however.
(or OTOH the wineserver would probably de-register hMenus in hWnds
"automatically", since it probably has access to hwnd data and can do it
itself)

> However, I don't think that this will fix the problem. I'm sure that my 
> patch won't break existing applications, and it's very important for 
> many Delphi applications, because Delphi often calls DestroyMenu on 
> active menus.
I'd also tend to move menu stuff into wineserver and then let wineserver
handle these issues.

A quick hack (that doesn't break existing applications, as possibly
mentioned on wine-devel around that time) may work and could be useful,
but proper wineserver menu support fixing all this at the same time
would be preferrable.

Andreas Mohr