Closed wxtrac closed 13 years ago
wxtrac
commented
16 years ago msw-uniquify-window-class-name.patch (1.7 KiB)Patch for uniquification of window class name for MSW
wxtrac
commented
16 years ago Thanks, this looks globally good but there are a couple of problems:
Thanks!
wxtrac
commented
16 years ago This Tracker item was closed automatically by the system. It was previously set to a Pending status, and the original submitter did not respond within 14 days (the time period specified by the administrator of this Tracker).
wxtrac
commented
16 years ago msw-uniquify-window-class-name-2.patch (7.8 KiB)Revised patch to address Vadim's concerns
wxtrac
commented
16 years ago File Added: msw-uniquify-window-class-name-2.patch
wxtrac
commented
16 years ago New patch attached to address Vadim's concerns.
wxtrac
commented
16 years ago Thanks, I've applied the patch with some changes:
I didn't fix the other remaining problem: there are other classes in wx (wxNotebook one at least) which are still not made unique. So we probably need to refactor this code further to make it more reusable...
Also, I'm not sure if this should be backported to 2.8 as in principle some code might be using ::FindWindow("wxWindowClass") but maybe it's not really a concern. In any case I prefer to keep it in trunk only for now to test it a bit more and backport it to 2.8 later if needed.
Thanks!
wxtrac
commented
14 years ago This patch breaks automated GUI testing. Here is why: Automated GUI testing (using AutomatedQA's TestComplete in our case) finds a control it tests by way of the controls WNDCLASS name using the ::FindWindow function. Before this patch, a control could be found with
["Window"]("wxWindowClassNR", "panel", 3)
In other words, find me the third window with a caption "panel" and the window class "wxWindowClassNR".
With this patch, the window class changes every time the application starts. IOW, none of our testscripts work anymore as the test robot no longer finds the controls in the application.
This affects all tools that use FindWindow to search for a specific window in a wxWidgets application, so it is likely that other tools than test robots will be affected as well.
Why exactly was this patch included? The classname is supposedly local to the module (.DLL/.EXE) registering it, so if two different modules register the same name, they should still receive their paint events correctly. What exactly was the problem? The description here is pretty vague.
I would like to reverse this patch or at least make this behaviour optional.
Regards
Hajo
wxtrac
commented
14 years ago Replying to [comment:6 hajokirchhoff]:
This patch breaks automated GUI testing.
I'm surprised the tool you use doesn't support some kind of wildcard matching because I believe MFC/ATL do something similar. At least I definitely see some randomly-looking numbers in their classes names.
Why exactly was this patch included? The classname is supposedly local to the module (.DLL/.EXE) registering it
Are you sure about this? I'm almost certain that all classes are application-global.
I would like to reverse this patch or at least make this behaviour optional.
We may make this behaviour optional, of course, but it shouldn't be reverted and it should remain on by default.
wxtrac
commented
14 years ago Replying to [comment:7 vadz]:
Are you sure about this? I'm almost certain that all classes are application-global.
Quoting MSDN:
Every window class needs a Class Name to distinguish one class from another. Assign a class name by setting the lpszClassName member of the WNDCLASSEX structure to the address of a null-terminated string that specifies the name. Because window classes are process specific, window class names need to be unique only within the same process. Also, because class names occupy space in the system's private atom table, you should keep class name strings as short a possible.
We may make this behaviour optional, of course, but it shouldn't be reverted and it should remain on by default.
What about using module's name instead of the semi-random pointer? That should still be unique within one process, while at the same time keeping the name predictable. We could check if such name is already registered, too, to make sure the conflict won't occur.
wxtrac
commented
14 years ago Replying to [comment:7 vadz]:
Replying to [comment:6 hajokirchhoff]:
This patch breaks automated GUI testing.
I'm surprised the tool you use doesn't support some kind of wildcard matching because I believe MFC/ATL do something similar. At least I definitely see some randomly-looking numbers in their classes names.
The SDK function ::FindWindow does not support wildcards or at least I didn't find it. Also, even though the tool might actually support wildcards for classnames, the patch still breaks all of our testscripts.
I would like to reverse this patch or at least make this behaviour optional.
We may make this behaviour optional, of course, but it shouldn't be reverted and it should remain on by default.
But why should it remain on by default? What was wrong with the old version? Why was this patch included?
Here [http://msdn.microsoft.com/en-us/library/ms633574%28VS.85%29.aspx#local] is the MSDN link to the documentation where it explicitly states that [quote] (Several modules can use the same name to register local classes in the same process. [/quote]
There is even an CS_GLOBALCLASS style to make the classname application global. IOW, this is not the default behaviour, you have to explicitly pass CS_GLOBALCLASS to RegisterWindow.
The way I understand this the only scenario where this patch would help was, if the wxWidgets library was statically linked to a module that already contained wxWidgets. IOW if it was statically linked against the wxWidgetsDLL itself or if the wxWidgets static library was included twice in a project. Only then would RegisterClass be called several times from within the same module and only then would a conflict occur. But I consider linking wxWidgets statically more than once a very unusual usage of the library.
BTW, adding the module name instead of a random number would not help in the "statically linked several times" scenario.
Regards
Hajo
wxtrac
commented
14 years ago I'm still not sure that using the same class name in 2 different modules inside the same process is not going to result in problems, even if neither of them uses CS_GLOBALCLASS. AFAICS, this flag makes it possible to create windows using this class but there is no unambiguous guarantee that the class names don't interfere in some way even if you can't create windows using them.
Of course, maybe I'm completely wrong and it's actually safe to reuse the class names, I don't know the details of the original problem neither. But the fact that the original submitter had some problems and that MFC does something similar (at least this is how I interpret it, why else would it use names such as "Afx:be0000:0:10005:10:0" (which does change between runs, too, e.g. it's "Afx:b40000:0:10005:10:0" if I relaunch Wordpad) for Wordpad ruler window?) argues for at least testing this carefully before reverting this change.
wxtrac
commented
14 years ago Replying to [comment:9 hajokirchhoff]:
BTW, adding the module name instead of a random number would not help in the "statically linked several times" scenario.
-Nothing* will help with this case, it's unsolvable.
wxtrac
commented
14 years ago Replying to [comment:10 vadz]:
Of course, maybe I'm completely wrong and it's actually safe to reuse the class names,
wxWidgets has been using the original method, without this patch, since 1999. See svn-revision 2504. It seems that the SDK documentation is correct and that it's safe to reuse the class name.
I don't know the details of the original problem neither. But the fact that the original submitter had some problems and that MFC does something similar
From the mfc sources: The method AfxRegisterWndClass registers a window class using a temporary name (wincore.cpp: 1408)
sprintf(... T("Afx:%p:%x:%p:%p:%p"), hInst, nClassStyle,
hCursor, hbrBackground, hIcon)The hInst part changes between runs.
The difference between our scenario and the MFC sources is that AfxRegisterWindowClass is meant for developers to be used directly, whereas our use is strictly internal.
My guess is they included the hInst in the classname to avoid having to check the CS_GLOBALAPP style. This way the names don't clash when the CS_GLOBALAPP style is set.
Summary from my point of view:
argues for at least testing this carefully before reverting this change.
Having wxWidgets working fine for 8 years without this patch is for me a strong indication that reverting this change won't cause a lot of problems.
Could we revert the patch in the trunc, leaving a comment refering to this trac item and see if someone reports problems with this?
Or make unique names optional using system options? I would set it to off by default. There exist other tools besides test robots, that use FindWindow. Macro recorders come to mind. All sort of tools that automate window interactions in some way.
Or we could introduce a global string variable - initially empty - that the developer could set himself that would be appended to the class names? That way, if someone really wants to link wxWidgets statically several times to the same module, he could set this variable first.
BTW (vaclav) adding the address of the pointer to the WNDCLASS name would indeed help in the case of "statically linked several times", as each image would be located at a different address, thus the pointer would be unique to each wxWidgets image in memory.
Regards
Hajo
wxtrac
commented
14 years ago Replying to [comment:12 hajokirchhoff]:
The difference between our scenario and the MFC sources is that AfxRegisterWindowClass is meant for developers to be used directly, whereas our use is strictly internal.
Err, how is that relevant?
The patch seems unnecessary according to the SDK documentation. SDK states explicitly: "Several modules can use the same name to register local classes in the same process".
The very same page contradicts itself by saying "window class names need to be unique [only] within the same process", so it's not 100% clear. The original submitter and MFC must have some reason to do this, too, and the wording of this patch's description makes it sound like it did actually fix something.
The little information we have on the original problem is that he seems to be trying to link wxWidgets in a very unusual way, such that the wxWidgets library image gets loaded several times into the modules address space.
No, the description says that the second wx image is in a DLL.
wxWidgets has been using non-uniquified names since 1999. If this was really a common problem, we should have heard about it long ago.
Quite a few bugs being fixed in wx were there for a long time, that doesn't mean they didn't exist. Loading 3rd party DLLs into a wx app is rare and having them use (a private copy of) wx is rarer still. That we didn't hear about such problems on MSW may as well mean that nobody tried doing that yet (until this patch was submitted). We didn't hear about these issues with OS X for a long time either.
Having wxWidgets working fine for 8 years without this patch is for me a strong indication that reverting this change won't cause a lot of problems.
This only tells us that the problematic usage is rare, so this "nobody complained for 8 years" argument doesn't help us. If, on the other hand, you wrote a small test case that demonstrated there's no conflict...
BTW (vaclav) adding the address of the pointer to the WNDCLASS name would indeed help in the case of "statically linked several times", as each image would be located at a different address,
I am not aware of any C++ linker that would be able to produce such binary. AFAIK you simply cannot have multiple correctly working wx images in the same module without doing some lowlevel magic, hence my unsolvability comment.
wxtrac
commented
14 years ago I wrote a test app to check this and in my testing, paint events appear to work correctly even without class name randomization.
Here's what I did: I patched wx trunk to use same classes as 2.8. When I load trunk-compiled DLL in an app built with wx 2.8 (both Unicode builds, running on XP, both using static wx builds), both parts work as expected. Both include at least one wxWindowClassNR window and the DLL handles wxEVT_PAINT in it.
wxtrac
commented
14 years ago Replying to [comment:14 vaclavslavik]:
I wrote a test app to check this and in my testing, paint events appear to work correctly even without class name randomization.
Thank you, that is good news.
So the SDK documentation is correct?
What do we do now with the patch?
wxtrac
commented
14 years ago Replying to [comment:15 hajokirchhoff]:
What do we do now with the patch?
Let's wait for some time if we hear anything from the original author of the patch but if there is no reply then we should indeed revert it, there is no sense in needlessly complicating the code.
wxtrac
commented
14 years ago Replying to [comment:16 vadz]:
Let's wait for some time if we hear anything from the original author of the patch but if there is no reply then we should indeed revert it, there is no sense in needlessly complicating the code.
Hi Vadim, Vaclav, 7 months have passed and no one has added anything to this discussion. I'm updating my program to the latest svn-head and the "uniquification" code is still in there.
So could you revert this patch, please? That would be great.
Thanks
Hajo
wxtrac
commented
14 years ago (In [65233]) Revert MSW window classes uniquification patch.
Making the class names unique doesn't seem to be necessary so revert the patch which appended unique pointer value to their names (57030).
See #9031.
wxtrac
commented
14 years ago The test above by vaclavslavik is wrong. The idea behind this patch is to have the statically-linked main app have its windows serviced by the main code, and the statically-linked DLL to have its windows serviced by the DLL code. The results appear to show that both are handled by the DLL, having registered the classes last. This is wrong and potentially disastrous.
Consider the case where the app is statically-linked with the 2.8 library and the DLL is statically-linked with the 2.9.2 library. Probably going to be crasharoony as the windows created by 2.8 in the main app are serviced by 2.9.2 code in the DLL. I didn't test this myself on the latest builds, but I had this exact problem in 2006(?) with different versions of the 2.6.x library. We had to hack in our own patch to uniquify the names until the patch we are discussing showed up at some point in the 2.8.x(?) tree.
Anyway, I understand the arguments about testing and the default version should definitely support automated testing. So instead of removing this patch as you've done in 2.9.2, please just add a #define flag around it and document it in the compiler configuration file. Or as suggested above, you could just add a string to MSW version of wxApp which by default would be empty, but would enable the programmer to explicitly uniquify the classnames as needed, without having to use compile-time flags (I like this option better).
Thanks for your consideration, -todd-
wxtrac
commented
14 years ago Replying to [comment:19 dsmtoday]:
The test above by vaclavslavik is wrong. The idea behind this patch is to have the statically-linked main app have its windows serviced by the main code, and the statically-linked DLL to have its windows serviced by the DLL code.
Please try to explain what's "wrong" in more detail, because your description matches the test I did.
The results appear to show that both are handled by the DLL,
No, it doesn't. 2.8 and 2.9 are too different for anything like that to work by accident.
Consider the case where the app is statically-linked with the 2.8 library and the DLL is statically-linked with the 2.9.2 library.
As I said, that's what I tested. And as a matter of fact, I use that setup in production with WinSparkle.
If you have a working example that demonstrates a problem with current code, please attach it (with instructions on how to compile and use it), so that we can reproduce the problem ourselves. As you said, you didn't test it recently, you just think that it probably doesn't work, so verifying and demonstrating that there actually is a problem would be the most useful thing now.
wxtrac
commented
13 years ago Someone else apparently believes this is needed, see #13315.
wxtrac
commented
13 years ago Replying to [comment:21 vadz]:
Someone else apparently believes this is needed, see #13315.
Yes, it was me... here is why
I use wxWidgets in my project (linked statically). I also load a 3-rd party DLL which also happens to use wxWidgets internally (linked statically). The two GUIs are totally independent. The first one which registers class will work OK. For the other one RegisterClass call will fail and thus wxWindowMSW::MSWGetRegisteredClassName() will always return NULL. It is, as far as I can tell, currently impossible to avoid this scenario without changing wxWidgets code, so I consider this a bug. Some compromise would be perfectly acceptable to me. Several come to mind:
a.) If RegisterClass() fails with error 'class already exists', only then try changing class name instead of returning NULL. b.) Allow the user to specify class name (or prefix, or suffix) instead of using hardcoded constant ('wxWidgets').
wxtrac
commented
13 years ago The fact that this change was made, then backed out, and now being discussed again 3 years later is a textbook example of how politically messed up open source projects can get.
I had a hard enough time just getting permission by someone to present a proposal for a draft of a plan to discuss a change for a possible bug fix the first time. Check the mailing list for all the gory details, including the counter argument that it just isn't necessary since no one should statically link wxWidgets. Or something. I never quite figured it out.
Good luck to you, juraj.ivancic et al. I've since completely moved away from wxWidgets.
wxtrac
commented
13 years ago I sincerely hope that this will not be the case. I read this discussion and I cannot see why this change was undone, except the erroneous 'code is unnecessarily complex' argument. I hope that my description of the problem will suffice, as creating a minimal example which demonstrates this error is non-trivial.
wxtrac
commented
13 years ago Please open up a new bug as I do not wish to receive further e-mails about this.
wxtrac
commented
13 years ago Replying to [comment:23 thomas_hauk]:
The fact that this change was made, then backed out, and now being discussed again 3 years later is a textbook example of how politically messed up open source projects can get.
This is a completely ridiculous accusation. There was nothing political about it, it was a purely technical discussion and the patch was reverted because applying it resulted in problems for other users (Hajo Kirchhoff, see comment:6).
I had a hard enough time just getting permission by someone to present a proposal for a draft of a plan to discuss a change for a possible bug fix the first time.
I have frankly no idea what are you speaking about.
Check the mailing list for all the gory details, including the counter argument that it just isn't necessary since no one should statically link wxWidgets. Or something. I never quite figured it out.
Reading this I can't figure out anything neither so I'm inclined to agree that you mustn't have understood it completely.
Anyhow, I've reread all the comments once again and AFAICS we still don't understand why does this problem happen in some cases and not the others. Vaclav wrote in comment:20 that he tested exactly the situation described in comment:22 and that it worked for him without any uniquification being needed. I admit that I don't quite understand why is it so, I'd expect this to fail because the same class can be only registered once inside a single process to the best of my knowledge but OTOH I didn't test this myself while Vaclav did. I hope somebody can explain why did it work for him or point out what did he do wrong or differently. And, as before, a simple test case allowing to reproduce the problem would still be very welcome.
Thanks!
wxtrac
commented
13 years ago Please open up a new bug. Thank you.
wxtrac
commented
13 years ago I'd prefer to keep the discussion here together with the old one. If you're not using wxWidgets at all any more you can probably just delete your account here (if you have trouble doing this please contact me privately).
wxtrac
commented
13 years ago register_class_bug.zip (180.4 KiB)wxtrac
commented
13 years ago I managed to create a small example which reproduces the error.
wxtrac
commented
13 years ago Replying to [comment:26 vadz]:
Anyhow, I've reread all the comments once again and AFAICS we still don't understand why does this problem happen in some cases and not the others. Vaclav wrote in comment:20 that he tested exactly the situation described in comment:22 and that it worked for him without any uniquification being needed. I admit that I don't quite understand why is it so, I'd expect this to fail because the same class can be only registered once inside a single process to the best of my knowledge
See the discussion above -- it is supposed to be unique within (module,process), not process, according to the documentation. But the very existence of this ticket as well as comment:29 suggest that that's not the whole story.
wxtrac
commented
13 years ago Ok, I think we can finally close this: the example doesn't work because you don't set the correct HINSTANCE for the DLL. Hence registering classes (and much else, probably) in it fails because it uses the app HINSTANCE instead.
Here is the diff needed to make your example work:
#!diff
--- app.cpp.orig 2011-07-07 22:50:48.000000000 +0200
+++ app.cpp 2011-07-11 17:25:39.642173400 +0200
@@ -19,17 +19,17 @@
class WxAppExe : public wxApp
{
public:
- typedef void (*RunWx)();
+ typedef void (*RunWx)(HINSTANCE);
virtual bool OnInit()
{
// Boom.
- wxFrame * frame = new wxFrame( NULL, wxID_ANY, "Frame", wxDefaultPosition, wxSize( 500, 500 ) );
+ wxFrame * frame = new wxFrame( NULL, wxID_ANY, "App", wxDefaultPosition, wxSize( 500, 500 ) );
frame->Show();
HMODULE test( ::LoadLibrary( L"test.dll" ) );
RunWx runWx = (RunWx)GetProcAddress( test, "runWX" );
- runWx();
+ runWx(test);
FreeLibrary( test );
return true;
}
--- dll.cpp.orig 2011-07-07 22:54:46.000000000 +0200
+++ dll.cpp 2011-07-11 17:25:43.189376300 +0200
@@ -1,5 +1,6 @@
#include <wx/frame.h>
#include <wx/app.h>
+#include <wx/msw/private.h>
#pragma comment( lib, "wxbase29ud.lib" )
#pragma comment( lib, "wxmsw29ud_core.lib" )
@@ -23,11 +24,12 @@
DECLARE_APP(WxAppDll);
IMPLEMENT_APP_NO_MAIN(WxAppDll);
-extern "C" __declspec(dllexport) void runWX()
+extern "C" __declspec(dllexport) void runWX(HINSTANCE hinstance)
{
int argc( 1 );
char * argv[] = { "Test", 0 };
wxInitializer initializer(argc, argv);
+ wxSetInstance(hinstance);
- new wxFrame( NULL, wxID_ANY, "Frame" );
+ new wxFrame( NULL, wxID_ANY, "DLL" );
};(the change of the frame titles is, of course, unnecessary and just allows to see better which frame is which).
I hope this takes care of your problems. I also wonder if we shouldn't check for valid HINSTANCE somewhere but I'm not totally sure if adding such checks is not going to break some existing working code...
And we definitely need to have a better way of setting it than using this semi-private wxSetInstance() function anyhow. We already have MSW-specific wxEntry() overload taking HINSTANCE, probably need to have a wxInitialize() one as well. [HowToSubmitPatches Patches] adding this would be welcome.
wxtrac
commented
13 years ago Adding wxSetInstance() call really did help. Thank you very much for looking into this.
Issue migrated from trac ticket # 9031
component: wxMSW | priority: low | resolution: invalid
2008-02-21 01:38:57: thomas_hauk created the issue
This patch uniquifies the window class name before registering it, similar to how wxCocoa does it. Allows paint events to get to windows generated by a second wx image in memory (e.g. statically linked to a DLL which is loaded).