Re: DestroyMenu: Check if the menu is still used (fix for bug 1486)
"Michael Kaufmann" <hallo(a)michael-kaufmann.ch> wrote:
Some applications call DestroyMenu for menus that are still displayed in a window. We need to check if a menu is still used and set the window's menu handle to NULL if that's the case.
This patch fixes the important bug 1486.
Changelog: - DestroyMenu: Check if the menu is still displayed and adjust the window's menu handle
There is a test case in Wine (dlls/user/tests/win.c,test_SetMenu) which shows that your patch is wrong. 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. -- Dmitry.
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: 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. My patch doesn't have the problem that he describes in the second post.
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? 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. Regards Michael
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
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.
Ulrich has a patch for this, but I think it's not finished.
Hi,
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.
It's nice to hear from you! If you succeeded, I wouldn't have this problem now ;-)
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.
I haven't found a comment about the revised patch. Comments about the first patch: "Menu patch breakage": http://www.winehq.org/hypermail/wine-devel/2002/08/0396.html "Re: Menu patch breakage": http://www.winehq.org/hypermail/wine-devel/2002/08/0399.html
My patch doesn't have the problem that he describes in the second post.
Hmm, the hMenu 0 problems? Why not?
If the menu is assigned to a window, it calls GetMenu to check if the window's menu has been changed (as your second patch does)
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.
Now I think too that my patch is a "quick hack", because it doesn't reset the menu handle of all windows displaying the menu. But reusing a menu does not work in WINE anyway. I'll try to fix that. Regards Michael
participants (4)
-
Andreas Mohr -
Dmitry Timoshkov -
Michael Kaufmann -
Mike Hearn