Student preparing for GSoC

[Kieran Duggan]

(This is a little long, but I wanted to show my steps so I could be
corrected)
So after spending a very long time looking at it (probably too long), I
think I have a solution.
Looking at the code snippet from /dlls/atl100/tests/atl.c

    hwnd = create_container_window();
    SetWindowLongW(hwnd, GWLP_USERDATA, 0xdeadbeef);
    container = NULL;
    hr = AtlAxAttachControl(control, hwnd, &container);
    ok(1==0, "Leaving AtlAxAttachControl second 0x%08x,
0x%08x\n",container, &container);
    ok(container != NULL, "Expected not NULL!\n");
    val = GetWindowLongW(hwnd, GWLP_USERDATA);
    ok(val == 0xdeadbeef, "Expected unchanged, returned %08x\n", val);
    DestroyWindow(hwnd);

The data that was never freed in this case was the container which got
assigned in the AtlAxAttatchControl function

==8187== 64 bytes in 1 blocks are definitely lost in loss record 271 of 577
==8187==    at 0x7BC519A5: RtlAllocateHeap (heap.c:260)
==8187==    by 0x4D27844: IOCS_Create (atl_ax.c:953)
==8187==    by 0x4D27844: AtlAxAttachControl (???:0)
==8187==    by 0x487D35A: func_atl (in
/home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/
atl100_test-stripped.exe.so)
==8187==    by 0x487D66E: ??? (in
/home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/
atl100_test-stripped.exe.so)
==8187==    by 0x487CDD8: main (in
/home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/
atl100_test-stripped.exe.so)
==8187==

The hard part of this for me was figuring out who was responsible for
freeing container. After some time I came to the conclusion that the caller
of
the AtlAxAttachControl function was intended to free container. I namely
came to this conclusion because I couldn't really see how the DestroyWindow
function would be able to free the container. If DestoyWindow is supposed
to be responsible for freeing container, I just wasn't
smart enough to figure it out and will have too look again.

To fix it, I just added  "IUnknown_Release(container);" after
DestroyWindow(hwnd) so the new code would look like this

    hwnd = create_container_window();
    SetWindowLongW(hwnd, GWLP_USERDATA, 0xdeadbeef);
    container = NULL;
    hr = AtlAxAttachControl(control, hwnd, &container);
    ok(1==0, "Leaving AtlAxAttachControl second 0x%08x,
0x%08x\n",container, &container);
    ok(container != NULL, "Expected not NULL!\n");
    val = GetWindowLongW(hwnd, GWLP_USERDATA);
    ok(val == 0xdeadbeef, "Expected unchanged, returned %08x\n", val);
    DestroyWindow(hwnd);
+  IUnknown_Release(container);

Valgrind shows that this does remove the memory leak
If this looks good enough, then I'll go ahead and submit it as a patch. I
just wanted to get opinions here before I did

Thank you all for your assistance up to this point. I've never worked with
a project as large as Wine so exploring the source
code and learning the debugging tools has been onerous, but I feel like
I've learned a lot and feel accomplished (even if this attempt is
incorrect).

On Sat, Mar 24, 2018 at 5:14 PM, Austin English <austinenglish at gmail.com>
wrote:

> On mobile, so can't verify, but look at:
> ==9846==    by 0x4B07572: AtlAxAttachControl (atl_ax.c:1156)
> Line 1156 in atl_ax.
> And
> ==9846==    by 0x4804025: test_AtlAxAttachControl (atl.c:902)
> Line 902 atl.c
>
> You can use `find -name atl.c` to find its location in the source checkout.
>
> On Sat, Mar 24, 2018, 13:52 Kieran Duggan <kieranduggan15 at gmail.com>
> wrote:
>
>> If that's the case, Is there a way that I can see where the
>> AtlAxAttachControl was called from? It's not necessary technically, but
>> knowing which test case caused a memory leak would be useful
>> When I ran the full test suite I got something like this
>>
>> ==9846== 64 bytes in 1 blocks are definitely lost in loss record 1,255 of
>> 2,380
>> ==9846==    at 0x7BC46859: notify_alloc (heap.c:260)
>> ==9846==    by 0x7BC49D56: RtlAllocateHeap (heap.c:1726)
>> ==9846==    by 0x4B06D40: IOCS_Create (atl_ax.c:952)
>> ==9846==    by 0x4B07572: AtlAxAttachControl (atl_ax.c:1156)
>> ==9846==    by 0x4804025: test_AtlAxAttachControl (atl.c:902)
>> ==9846==    by 0x4805130: func_atl (atl.c:1092)
>> ==9846==    by 0x4805485: run_test (test.h:603)
>> ==9846==    by 0x4805EDC: main (test.h:687)
>> ==9846==
>>
>> Because I can see what parameters the method was called with I get a
>> little more useful information
>> For example this one shows that a specific test case is where the
>> function causes
>> If a leak f I look around the logs a little more I don't see the other
>> test cases breaking.
>> So if there is a way to see where AtlAxAttachControl is being called from
>> while running the test that would be useful.
>>
>> On Sat, Mar 24, 2018 at 2:49 AM, Austin English <austinenglish at gmail.com>
>> wrote:
>>
>>> On Sat, Mar 24, 2018 at 1:34 AM, Kieran Duggan <kieranduggan15 at gmail.com>
>>> wrote:
>>> > So I'm attempting to run individual tests now. What exactly should I do
>>> > after I "make service.ok"? I tried just running valgrind the way I
>>> would
>>> > expect to use it i.e.
>>> >
>>> > valgrind --trace-children=yes --track-origins=yes --leak-check=full
>>> > --num-callers=20 --workaround-gcc296-bugs=yes --log-file="filename.log"
>>> > ./wine
>>> > /home/kduggan15/Documents/Wine/wine-valgrind/dlls/atl100/tests/
>>> atl100_test-stripped.exe.so
>>> >
>>> > I get
>>> >
>>> > 64 bytes in 1 blocks are definitely lost in loss record 1,250 of 2,380
>>> > ==11711==    at 0x7BC46859: notify_alloc (heap.c:260)
>>> > ==11711==    by 0x7BC49D56: RtlAllocateHeap (heap.c:1726)
>>> > ==11711==    by 0x4D16D40: IOCS_Create (atl_ax.c:952)
>>> > ==11711==    by 0x4D17572: AtlAxAttachControl (atl_ax.c:1156)
>>> > ==11711==    by 0x4804025: ??? (in
>>> > /home/kduggan15/Documents/Wine/wine-valgrind/dlls/atl100/tests/
>>> atl100_test-stripped.exe.so)
>>> >
>>> > The issue being that the atl100_test-stripped.exe.so doesn't have any
>>> debug
>>> > symbols
>>> > Also I initially tried make service.ok and I just get something
>>> likemake
>>> > service is up to date or it just builds the tests without running
>>> valgrind.
>>> > If it does run valgrind, then I don't know where the log is going (and
>>> I
>>> > checked wine-valgrind/logs of course)
>>> >
>>> > That's basically all I got. Sorry if these questions are a bit basic.
>>> I'm
>>> > just running into a few roadblocks and could use the assistance. I
>>> really
>>> > appreciate the help
>>>
>>> Hi Kieran,
>>>
>>> You've got it. Notice how the output is prefixed with the pid of the
>>> wine process; that's valgrind's output. The source code lines on the
>>> end of the line, like a backtrace.
>>>
>>> So that output shows that there's a leak in atl100, allocated via
>>> heap.c's notify_alloc, then RtlAllocateHeap, then IOCS_Create, then
>>> AtlAxAttachControl).
>>>
>>> Your task would be to find that leak, then fix it, i.e., free the
>>> memory where it should have been, but wasn't.
>>>
>>> FYI, after running the tests, you may need to run 'rm service.ok',
>>> otherwise the test won't run (only if it succeeded beforehand).
>>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20180326/34530068/attachment.html>