Not all TestSuites are processed correctly

hoppfrosch commented 11 years ago

I've started to use your framework more and more:

at the moment I've got the following:

#Include %A_ScriptDir%\Yunit\Yunit.ahk
#Include %A_ScriptDir%\Yunit\Window.ahk
#Include %A_ScriptDir%\Yunit\StdOut.ahk
#include <WindowHandler>

#Warn All
#Warn LocalSameAsGlobal, Off
#SingleInstance force

Yunit.Use(YunitStdOut, YunitWindow).Test(MiscTestSuite, HideShowTestSuite, PropertiesTestSuite, ExistTestSuite, RollupTestSuite)
Return

class PropertiesTestSuite 
{
    Begin()
    {
        Global debug
        this.obj := new WindowHandler(0, debug)
    }

    SetGetRemoveProperty() {
        val := 1
        ; Set the Property
        this.obj.setProp("wh_Test", val)
        ; Fetch the property again
        ret := this.obj.getProp("wh_Test")
        Yunit.assert(ret == val)
        ; Remove the Property
        ret := this.obj.removeProp("wh_Test")
        Yunit.assert(ret == 1)
    }

    GetNonExistingProperty() {
        ; Remove the Property in case it exists ...
        ret := this.obj.removeProp("wh_Test")
        ; Fetch the property again
        ret := this.obj.getProp("wh_Test")
        Yunit.assert(ret == 0)
    }

    End()
    {
        this.obj.kill()
        this.remove("obj")
        this.obj := 
    }
}
...

I do have 5 TestSuiteClasses which do a few tests each (some simple example see above)

BUG: Only the first four of the five TestSuiteClasses are called from Yunit.Use(YunitStdOut, YunitWindow).Test(MiscTestSuite, HideShowTestSuite, PropertiesTestSuite, ExistTestSuite, RollupTestSuite) (RollupTestSuite is not called in the example)

Putting RollupTestSuite as first argument in the use-call, only RollupTestSuite and MiscTestSuite are called (2 out of 5) ...

It seems there's still something wrong calling the testsuites ...

infogulch commented 11 years ago

Thanks for letting us know.

Could you either post the entire test suite or a minimal test case that reproduces the behavior? (use a gist if you like)

hoppfrosch commented 11 years ago

I'll try to isolate from my current dev - and send it ...

Here it is: https://gist.github.com/hoppfrosch/636985f76fdb2ab32542

I wasn't able to produce a smaller testcase. This is an excerpt of my current work in progress - so be indulgent with my code ... ;-)

Note: 6 Testsuites are defined (RollupTestSuite, MiscTestSuite, HideShowTestSuite, PropertiesTestSuite, ExistTestSuite)

At my machine only two testsuites are processed RollupTestSuite, MiscTestSuite.

Changing the order of testsuites gives different results.Each order gives a reproducable amount of executed testsuites ...

infogulch commented 11 years ago

Damn... pulls out giant DEL hatchet

infogulch commented 11 years ago

Ok, strange things. Before I changed anything, I get these results:

Running in unicode 64-bit looks like it goes through all the tests, but then AutoHotkey crashes at the end.
After changing to unicode 32-bit it exhibits the behavior you mentioned (only running two of the test suites)

(Also wtf happened to notepad++ folding? It's totally fubar.)

infogulch commented 11 years ago

So this is the smallest test case I can make that exhibits the erroneous behavior. Specifically, it runs RollupTestSuite and BBB test suite, but fails to call any tests in CCC test suite. It requires the WindowHandler file that @hoppfrosch pasted in the gist above.

#Include %A_ScriptDir%\Yunit\Yunit.ahk
#Include %A_ScriptDir%\Yunit\Window.ahk
#include <WindowHandler>

#SingleInstance force

Yunit.Use(YunitWindow).Test(RollupTestSuite, BBB, CCC)
Return

class RollupTestSuite
{
    TEST()
    {
        this.obj := new WindowHandler(0, 0)
        this.remove("obj")
    }
}

class BBB
{
    TEST()
    {
        this.obj := new WindowHandler(0, 0)
    }
}

class CCC
{
    TEST()
    {
    }
}

If any of the 3 remaining lines inside the test suites are removed it behaves correctly. (Adding additional this.obj.kill() or this.remove("obj") has no effect on this issue.) If the name RollupTestSuite is changed it behaves normally. (wtf!!!)

I'll see if I can nuke the WindowHandler file small enough to paste.

infogulch commented 11 years ago

Ok, I nuked WindowHandler.ahk, and found this is the smallest amount of code that still reproduces the bad behavior. (Ok, maybe not the smallest possible, but my DEL key was begging for mercy so I stopped.) I only removed lines, I didn't change them (for the most part).

class CONST_EVENT
{
    class OBJECT
    {
        static CONTENTSCROLLED      := 0x8015
    }
}

ClassWindowHandler_EventHook(hWinEventHook, Event, hWnd, idObject, idChild, dwEventThread, dwmsEventTime )
{
    Object(A_EventInfo)
}

class WindowHandler
{
    __SetWinEventHook(eventMin, eventMax, hmodWinEventProc, lpfnWinEventProc, idProcess, idThread, dwFlags)
    {
        ret := DllCall("ole32\CoInitialize", Uint, 0)
        ret := DllCall("user32\SetWinEventHook"
            , Uint,eventMin   
            , Uint,eventMax   
            , Uint,hmodWinEventProc
            , Uint,lpfnWinEventProc
            , Uint,idProcess
            , Uint,idThread
            , Uint,dwFlags)   
        return ret
    }

    __New(_hWnd=-1, _debug=0, _test=0)
    {
        Run, notepad.exe
        WinWait, ahk_class Notepad, , 2
        WinMove, ahk_class Notepad,, 10, 10, 300, 300
        _hWnd := WinExist("ahk_class Notepad")
        this._hWnd := _hWnd

        this._HookProcAdr := RegisterCallback("ClassWindowHandler_EventHook", "", "", &this)
        this._hWinEventHook2 := this.__SetWinEventHook( CONST_EVENT.OBJECT.SHOW, CONST_EVENT.OBJECT.CONTENTSCROLLED, 0, this._HookProcAdr, 0, 0, 0 )
    }
}

Interestingly, if the one line in ClassWindowHandler_EventHook is removed, Yunit behaves correctly.

@hoppfrosch at the moment it looks like the problem lies outside of Yunit. Specifically, you should probably be double checking that A_EventInfo is a valid address before getting an object from it. (Perhaps a callback is being made after the object is destroyed.)

Uberi commented 11 years ago

My guess would be use after free issue solved using objaddref (Yunit should probably make the programmer's life easier by making a fake reference), but won't be able to test until I get to a computer, at latest tomorrow. Great detective work infogulch.

Uberi commented 11 years ago

Debugging: adding ObjAddRef(&this) before the RegisterCallback line fixed crasher, but created memory leak. Memory leak is insignificant since only a small and finite number of WindowHandler objects will be created during a given program run. Is this a workable solution?

hoppfrosch commented 11 years ago

Thanks for investigating my code and pointing this out. At the moment I'll be OK with this. WindowHandler is planned to be core of an application which allows to give all windows a window history which can be rolled back. The number of windows will be the number of open windows during a session. Don't know whether this can be called "only a small and finite number" ....

As I'm not the biggest expert: The memory leaks should be avoidable if an ObjRelease(this) is added to __Delete(). Is this assumption true?

Edit: Adding the suggested ObjAddRef(&this) also resolved other issues I had with properly detecting resized or moved windows ... Thanks, thanks, thanks!

Uberi commented 11 years ago

No, the problem with using ObjRelease is that __Delete will never get called at all due to the ghost reference. It might work if the object is released when the window is destroyed, though. I think a shell hook can detect that.

hoppfrosch commented 11 years ago

I'm trying to understand in depth what happens here, but still can't ...

@Uberi wrote - but I did not get the sense of this sentence:

My guess would be use after free issue solved using objaddref (Yunit should probably make the programmer's life easier by making a fake reference)
What causes the memory leak? Is it the added reference (ObjAddRef(&this)), which cannot released properly?
Is the added reference (ObjAddRef(&this)) the ghost reference you talk about? Do not know the term ghost reference yet ...
Why is __Delete not called? Within the constructor of my object I call ObjAddRef(&this) - destroying the object should call the destructor (__Delete()) and therefore ObjRelease(&this) should be called also (that's what I thought I understood so far ...)
How does a shell hook for this look like? Uberi wrote:

I think a shell hook can detect that.
Where does the whole problem come from? Is it a principal problem in these cases (so I cannot avoid it) - or can it be avoided taking another approach to the whole thing? How can another approach look like? Basically I want to implement a event based object, which can react on certain events ... (I want to give a single window a history of its resizing/moving actions: each resize/movement should be stored ...)

Thanks a lot for your patience and mentorship!

Uberi commented 11 years ago

AHK uses a garbage collection mechanism known as reference counting. In short, each object keeps track of how many references there are that point to it, and when this number reaches 0, the object is "garbage" and can have its memory freed.

In our case, WindowHandler was prematurely freed when it was not stored anywhere in any variables or objects (not being referenced), but we still wanted to use it (Object(A_EventInfo) in the event handler). This means we're using memory that's been freed, which caused the crashing. Using ObjAddRef(&this) incremented the reference count by 1, so that when all references to the object are removed, the count is 1 rather than 0, so the object is not freed. Obviously, since the count is never 0, the __Delete method is never called. I call the extra 1 the "ghost" or fake reference, since it acts like a reference but does not actually exist.

The problem with this is that now the object's reference count cannot reach 0, even if all references are removed. That means the memory taken by the object isn't freed when we're done with it, so as we create more WindowHandler objects, more memory is used that cannot be reclaimed.

What needs to be done is to look at the object itself - when do we stop needing it? Well, in our case, when the window is closed, the WindowHandler can be removed. To allow it to be removed, we remove the ghost reference so that the reference count can reach 0 again, and the object can be freed. This is done using ObjRelease(&this) when the window is closed.

Finally, we can detect when the window is closed using a shell hook.

However, if you ask me, the best solution is to remove the event hook when the WindowHandler object is freed, so that the event handler is not called when the WindowHandler object does not exist. I believe you can do that using UnhookWinEvent in the __Delete method of WindowHandler, and removing the ObjAddRef and ObjRelease hackery.

infogulch commented 11 years ago

However, if you ask me, the best solution is to remove the event hook when the WindowHandler object is freed, so that the event handler is not called when the WindowHandler object does not exist. I believe you can do that using UnhookWinEvent in the __Delete method of WindowHandler, and removing the ObjAddRef and ObjRelease hackery.

I agree with this.

But looking through the code hoppfrosch pasted shows that he is using UnhookWinEvent. See his paste. hoppfrosch, perhaps you should be checking the return value to make sure the call is successful.

Uberi commented 11 years ago

Ah, didn't notice that, I guess the unhooking call might be failing...

hoppfrosch commented 11 years ago

Thanks for your elaborated instructions.

I'll get the return value of my UnhookWinEvent calls - but what can I do if it fails (beside repeatedly trying)?

Uberi commented 11 years ago

I don't think it's supposed to fail very often, it might be caused by something in the code, perhaps.

Uberi / Yunit

Not all TestSuites are processed correctly #5