Am Samstag, den 31.05.2008, 12:08 +0200 schrieb Jens Albretsen: The patch looks like it would work as advertised, but the following snippet in the testcase smells fishy:
+static unsigned long getRefcount(IUnknown *iface) +{
- IUnknown_AddRef(iface);
- return IUnknown_Release(iface);
+}
[...]
- IDirectDraw_Release(lpDD);
- ref = getRefcount(lpDD);
- ok(ref == 0, "Got refcount %ld, expected 0\n", ref);
- IUnknown_Release(surface);
The IDirectDraw_Release seems to be dropping the reference count of the DirectDraw object to zero. This is OK, of course, as you might well drop the last reference. But this might put the DirectDraw object into a state where AddRef crashes, as parts of the object might get destroyed when the reference count drops down to zero.
On wine, there is obviously no problem, as wine keeps the DirectDraw object itself alive for longer. On Windows, it is not clear to me that the DirectDraw object is not destroyed at the IDirectDraw_Release, the only thing we know is, that Direct Draw still is working (perhaps implemented by a subobject of the DirectDraw object which keeps living).
I suggest to alternatively write the second part as + ref = IDirectDraw_Release(lpDD); + ok(ref == 0, "Got refcount %ld, expected 0\n", ref); + + IUnknown_Release(surface);
Regards, Michael Karcher
On Saturday 31 May 2008 14:57:25 Michael Karcher wrote:
Am Samstag, den 31.05.2008, 12:08 +0200 schrieb Jens Albretsen: The patch looks like it would work as advertised, but the following
snippet in the testcase smells fishy:
+static unsigned long getRefcount(IUnknown *iface) +{
- IUnknown_AddRef(iface);
- return IUnknown_Release(iface);
+}
[...]
- IDirectDraw_Release(lpDD);
- ref = getRefcount(lpDD);
- ok(ref == 0, "Got refcount %ld, expected 0\n", ref);
- IUnknown_Release(surface);
The IDirectDraw_Release seems to be dropping the reference count of the DirectDraw object to zero. This is OK, of course, as you might well drop the last reference. But this might put the DirectDraw object into a state where AddRef crashes, as parts of the object might get destroyed when the reference count drops down to zero.
On wine, there is obviously no problem, as wine keeps the DirectDraw object itself alive for longer. On Windows, it is not clear to me that the DirectDraw object is not destroyed at the IDirectDraw_Release, the only thing we know is, that Direct Draw still is working (perhaps implemented by a subobject of the DirectDraw object which keeps living).
Someone please run the test on windows, I have no windows installation.
I suggest to alternatively write the second part as
- ref = IDirectDraw_Release(lpDD);
- ok(ref == 0, "Got refcount %ld, expected 0\n", ref);
- IUnknown_Release(surface);
It would pretty much kill the idea of the test.
The game demo does this, and runs in windows. I'm trying to emulated the behavior of windows, it fails miserably in wine as the ddraw object suddenly disappears. I can only see what the demo tries to do and make a fix for that, since I have no way of checking it in windows myself. Anyway it makes wine stop crashing when programs tries to do something crazy like this. Stefan Dösinger asked me to put a HeapAlloc all memory between the two Release's to check if the memory of ddraw object would be corrupt in Windows. It really doesn't matter if programs live by chance in windows, if we can implement it so it's not by chance anymore. Isn't that better.
Regards, Michael Karcher
Cheers, Jens Albretsen
Am Samstag, den 31.05.2008, 15:43 +0200 schrieb Jens Albretsen:
On wine, there is obviously no problem, as wine keeps the DirectDraw object itself alive for longer. On Windows, it is not clear to me that the DirectDraw object is not destroyed at the IDirectDraw_Release, the only thing we know is, that Direct Draw still is working (perhaps implemented by a subobject of the DirectDraw object which keeps living).
Someone please run the test on windows, I have no windows installation.
I will do that, no problem.
I suggest to alternatively write the second part as
- ref = IDirectDraw_Release(lpDD);
- ok(ref == 0, "Got refcount %ld, expected 0\n", ref);
- IUnknown_Release(surface);
It would pretty much kill the idea of the test.
OK, I jumped too quickly to my conclusion. In the ddraw log posted to the bug, the reference count really drops down to zero and exactly that object is used afterwards. Counting the QueryInterface and Release calls, I can't understand why the reference count drops to zero.
I will investigate further and respond back then.
Anyway it makes wine stop crashing when programs tries to do something crazy like this. Stefan Dösinger asked me to put a HeapAlloc all memory between the two Release's to check if the memory of ddraw object would be corrupt in Windows. It really doesn't matter if programs live by chance in windows, if we can implement it so it's not by chance anymore.
I agree on that point.
Cheers, Jens Albretsen
Regards, Michael Karcher
Am Samstag, 31. Mai 2008 17:03:13 schrieb Michael Karcher:
Anyway it makes wine stop crashing when programs tries to do something crazy like this. Stefan Dösinger asked me to put a HeapAlloc all memory between the two Release's to check if the memory of ddraw object would be corrupt in Windows. It really doesn't matter if programs live by chance in windows, if we can implement it so it's not by chance anymore.
I agree on that point.
Actually, that is dangerous. There may be situations in which this make-something-work has an unintended side effect, e.g. failure to restore the display mode because the ddraw object isn't destroyed(although it maybe should be - it's the test's job to find that out).
Another possibility are apps that intend to cause an exception, like copy protection systems.
-
Jens Albretsen -
Michael Karcher -
Stefan Dösinger