Sign in to follow this  
gretty

HINSTANCE & HWND Corrupted when passed around functions

Recommended Posts

Hi, I have a funny error occurring at runtime where my HINSTANCE and HWND variables are getting corrupted. I have posted the debugging output below and the line of code where the runtime error occurs. *Note: even though the screen capture kindof shows the HWND isn't corrupted, it doesn't point to the right/valid window anymore

Why is this problem occurring and how can I fix it?
 

WindowTiler.exe!WindowLayoutComponent::init(const IEventArgs & evtArgs) Line 26    C++  
[External Code]      
WindowTiler.exe!EventDelegate::operator()(const IEventArgs & evtArgs) Line 21    C++  
WindowTiler.exe!IApp::eventHandler(const int & evtId, const IEventArgs & evtArgs) Line 20    C++  
WindowTiler.exe!Win32App::wndProc(HWND__ * hwnd, unsigned int message, unsigned int wParam, long lParam) Line 18    C++  
[External Code]      
[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]      
WindowTiler.exe!Win32App::initInstance(const Win32AppInit & evtArgs) Line 154    C++  
WindowTiler.exe!Win32App::init(const IEventArgs & evtArgs) Line 100    C++  
WindowTiler.exe!App::init(const IEventArgs & evtArgs) Line 45    C++  
WindowTiler.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 16    C++  
[External Code]      

 

Status WindowLayoutComponent::init(const IEventArgs& evtArgs)
{
    auto args = (const WinEventArgs&)evtArgs;

    HWND btn = CreateWindowEx(WS_EX_TRANSPARENT, _T("Button"), _T("Test"), WS_VISIBLE | WS_CHILD | WS_EX_TRANSPARENT,
        30, 30, 50, 50, args.hwnd, NULL, args.hinstance, 0); // LINE 26

    return S_SUCCESS;
}

k0nbW.png

Edit: There are 2 IEventArgs objects involved. One passed to App::init(const IEventArgs & evtArgs) and another different object created in Win32App::wndProc and passed to IApp::eventHandler(const int & evtId, const IEventArgs & evtArgs).
 

LRESULT CALLBACK Win32App::wndProc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
{
    int wmId, wmEvent;
    PAINTSTRUCT ps;
    HDC hdc;

    WinEventArgs args { hinstance, hwnd, wParam, lParam };
    eventHandler(message, args);
    ...

 

Share this post


Link to post
Share on other sites

I've applied the advice given; remove c-style casts, explicitly specify the variable type instead of using auto and resulting in a copy. Unfortunately the crash still occurs when accessing the args object.

 

The "args" parameter has "unable to read memory" showing up in the wParam. This appears to be that your args value is broken, not just the hhnstance or hwnd parameters.

Yes it looks like my args value is broken. Do you have any advice on how I can track down what in my code or design I have done wrong to cause this? I am correctly passing an object of type WinEventArgs to the Win32App::eventHandler() so the problem isn't that I am passing a different subclass IEventArgs.

 

Any advice on how to track down the cause would be greatly appreciated.

Share this post


Link to post
Share on other sites

You said you "explicitly specify the variable type." Are you specifying it _as a reference_? Code, real errors, and not screenshots.

 

If you know where the eventArgs is good in your call chain, look at each function in the callstack and inspect the value being passed around. Since you're expecting a reference, check the address too. See where it starts going wrong. That spot (or the caller) is probably where your bug lies.

Share this post


Link to post
Share on other sites

I am specifying a reference; const WinEventArgs& args = static_cast<const WinEventArgs&>(evtArgs);

 

One thing that could be the cause is that; LRESULT CALLBACK Win32App::wndProc(..) is a static function. Therefore when it creates the WinEventArgs object it is also static aswell. The last time the WinEventArgs object is valid is inside EventDelegate::operator()(const IEventArgs & evtArgs) - not a static function.

// Static class function
LRESULT CALLBACK Win32App::wndProc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
{
    int wmId, wmEvent;
    PAINTSTRUCT ps;
    HDC hdc;

    WinEventArgs args { hinstance, hwnd, wParam, lParam };
    eventHandler(message, args);  // also static function: IApp::eventHandler(const int & evtId, const IEventArgs & evtArgs)
    ...

Share this post


Link to post
Share on other sites

If the function is being called after the lifetime of your `args` value ends, then yeah that would do it. Being static or not is irrelevant; all that matters is lifetime.

 

Since you're asking about ways to diagnose this, though, just look at your debugger. At least from what I can guess from your callstack, lifetime is not the issue. Just actually look at the frames in question and inspect the values at each frame. The debugger has all the information you need, you just have to look at it. :)

Share this post


Link to post
Share on other sites

how is hinstance declared? Since wParam is not used by the function where the error is occurring, and hwnd appears to be valid, I think hinstance is the issue here.

also, while WPARAM is defined as a UINT_PTR, wParam should actually be treated as an integer for many messages.

Share this post


Link to post
Share on other sites

After extensive debugging I have found the problem and solved it. It's actually quite interesting what went wrong. The problem was here...

struct IEventArgs
{

};

struct WinEventArgs : public IEventArgs
{
    WinEventArgs() = delete; 
    WinEventArgs(const HINSTANCE& hinstance, const HWND& hwnd, const WPARAM& wParam, const LPARAM& lParam) :
        hinstance(hinstance), hwnd(hwnd), wParam(wParam), lParam(lParam)
    {}

    const HINSTANCE& hinstance;
    const HWND& hwnd;
    const WPARAM& wParam;
    const LPARAM& lParam;
};

class EventDelegate
{
public:
        // the problem is here: the parameter should be 'const IEventArgs&'
	typedef std::function<Status(IEventArgs)> EDelegate;

	EventDelegate(EDelegate delegate, GUID gUidContext);

	Status operator()(const IEventArgs& evtArgs)
        {
            return delegate(evtArgs);
        }

private:
	GUID gUidContext;
	EDelegate delegate;
};

I've produced a simple example of the problem that people can reproduce. Whilst I've fixed it, what exactly was going wrong, was it that a copy was being made and I hadn't implemented a copy constructor?

struct IEventArgs {  };

struct WinEventArgs : public IEventArgs
{
	WinEventArgs(const int& a, const int& b) : a(a), b(b) { }
	const int& a;
	const int& b;
};

class Component
{
public:
	void test(const IEventArgs& evtArgs)
	{
		const WinEventArgs& args = static_cast<const WinEventArgs&>(evtArgs);
		printf("a=1 is: %d, b=2 is: %d\n", args.a, args.b);
	}
};

int main()
{
	WinEventArgs args(1,2);
	Component cmp;
	
	// Note Component::test's parameter is 'const IEventArgs&' and that the std::function parameter is just 'IEventArgs'
	std::function<void(IEventArgs)> func = std::bind(&Component::test, cmp, std::placeholders::_1);
	func(args);
	
	// When std::function parameter is 'const IEventArgs&' the cast inside Component::test succeeds
	std::function<void(const IEventArgs&)> func2 = std::bind(&Component::test, cmp, std::placeholders::_1);
	func2(args);
	
	return 0;
}
Edited by gretty

Share this post


Link to post
Share on other sites
It seems to be a slicing of WinEventArgs due to a permissive interface. At the time of passing in [tt]args[/tt] to [tt]func[/tt], an assignment from [tt]const WinEventArgs&[/tt] to [tt]IEventArgs[/tt] is made. Your callback then receives this copy and erroneously downcasts to WinEventArgs, and accesses invalid memory.

So the first step is to lock down your interface:
// How do I C++?
struct IEventArgs {
protected:
	IEventArgs() = default;
	~IEventArgs() = default;

public:
	IEventArgs(const IEventArgs&) = delete;
	IEventArgs& operator = (const IEventArgs&) = delete;
};
The next step is to stop using const references on small data types. Not only will you need to be extremely mindful of the lifetimes of the objects they refer to, but you also incur double-indirection.
 
struct WinEventArgs : public IEventArgs
{
	WinEventArgs(const int a, const int b) : a(a), b(b) { }

	const int a;
	const int b;
};
The general rule of thumb is to pass-by-reference objects that are expensive to copy.
POD-types (int, float, WPARAM, LPARAM) and handles (such as HINSTANCE, HWND) are all integral-types. Pass-by-value*.
A vector<int> with 25000 elements? Pass-by-reference.


EDIT:
With the updated code above, Visual Studio reports:
std::function<void(IEventArgs)> func = std::bind(&Component::test, cmp, std::placeholders::_1);
func(args);  // C2280: 'IEventArgs::IEventArgs(const IEventArgs &)': attempting to reference a deleted function
Edited by fastcall22

Share this post


Link to post
Share on other sites

I agree with fastcall22, there's no point to having the members of WinEventArgs being references, they are POD value types and you gain no benefit to pass by reference. I would keep them const, as these values should not be changed in processing.

Yes, because your EDelegate takes the IEventArgs by value, it performs a copy construction of IEventArgs from your WinEventArgs - the WinEventArgs data is lost here. This IEventArgs copy is then passed by reference to the function wrapped by EDelegate.

making IEventArgs non-copyable, as fastcall demonstrated, is the simplest way to prevent this mistake from recurring.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this