[dplayx 02/29] Tests for DirectPlayCreate

[Michael Karcher]

Am Donnerstag, den 10.07.2008, 22:04 +0300 schrieb Ismael Barros:
> >> >> +    heap = HeapCreate( 0, 0, 0 );
> >> > Do you have any reason to create your own heap? Each windows process
> >> > already comes equipped with a standard heap you can obtain using the API
> >> > function GetProcessHeap, so this line:
> >> >> +    IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
> >> > would then look like:
> >> > +    IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
> >> 
> >> That's what I used in the begining, but I've got some HeapAllocs that
> >> I don't free.
> > OK. That's a point. I didn't expect it, though. I would like a comment
> > in there along
> >   /* Create an extra heap to throw away garbage after tests */
> No problem.
[...]
> Hm, yeah, I thought about a cyclic buffer but wanted to keep the code simple.
> However this heap thing is creating some skepticism, so I'll go that way.
For me, the create-heap-and-throw-away approach is OK, but as it is not
the way it is usually handled in wine, I would like to see a comment.
Perhaps AJ prefers cyclic buffers or throw-away heaps. Or perhaps he
doesn't care how it is done in test cases.

> > But in my opinion, testing of the reaction to contract violation should
> > not go into passing somehow valid-looking pointers, that do not point to
> > anything sensible. And the main point was, that I really don't get why
> > it must be a heap pointer. The an LPDWORD on the stack would do as well
> > as allocating sizeof(LPDWORD) area.
> I've been checking how this is done in other tests, and it's
> particularly interesting how they do it in ddraw/tests/refcount.c:81:
>   hr = IDirectDraw7_CreatePalette(DDraw7, DDPCAPS_ALLOW256 | DDPCAPS_8BIT, Table, &palette, (void *) 0xdeadbeef);
I don't think this line is good style, but I still like it more than
your program, as this will definitely crash if the pointer is
derefenced, while your variant makes the function derefencing the
pointer accessing uninitialized data.

> This is the only example in the current code. I could either do it
> that way (not correct, but simple), or use some IUnknown interface. I
> tried to get the IUnknown of the classes I have access to, DirectPlay
> or DirectPlayLobby, but the current implementation is not able to
> return an interface for IID_IUnknown (it just fails with
> E_NOINTERFACE),
Thats bad. Every COM object must return a valid interface pointer for
IID_IUnknown. And even worse: The IID_IUnknown pointer *has* to be
stable. So if you do the following (pretend that CreateFooBar(REFIID,
DWORD, IUnknown*) is a factory function:

  IFooBar *fb1, *fb2;
  IUnknown *unk1, *unk2;
  fb1 = CreateFooBar(IID_IFooBar, 0, NULL);
  IFooBar_QueryInterface(fb1, IID_IUnknown, &unk1);
  IFooBar_QueryInterface(fb1, IID_IFooBar, (IUnknown*)&fb2);
  IFooBar_Release(fb1);
  IFooBar_QueryInterface(fb2, IID_IUnknown, &unk2);
  IFooBar_Release(fb2);
  ok(unk1 == unk2, "COM violation: IUnknown interface must be stable!");
  IUnknown_Release(unk1);
  IUnknown_Release(unk2);

The COM specification guarantees the test to succeed, independent of
what type IFooBar has! DirectPlay currently has no permanent COM object,
but creates a new object on the fly in QueryInterface; this is needed to
support the different vtables.

> so I would have to fix that too (and I'm not very sure about how to do
> it, without breaking anything. Would returning any of the existing
> interfaces that have CreateInstance, AddRef and Release, be correct?
If they were permanent, it would be OK. All COM interfaces are derived
from IUnknown, and can thus be used as IUnknown. The only catch is the
object lifetime uniqueness of the IUnknown interface.

>  This seems to be done a lot in other dlls)
For ddraw for example, it is correct. The IDirectDrawImpl object is live
until all references to it (including the IUnknown one) are gone.

Regards,
  Michael Karcher