Refcounting in Direct3DDevice8::Release (again)

[CookiePLMonster]

First of all, do note I don't have this issue backed by anything but theory.

Something that has been bugging me out for a while - feels like if (myRef <= 3) might not be safe in all cases and sometimes it may leave device in semi-corrupted case. To visualise this, let's consider three cases:

1. Both _current_rendertarget and _current_depthstencil are not null: Device wrapper has 3 references left when there is one reference left in user code (because the other two come from rendertarget and depthstencil) - they are released once the last pointer from user code is released. Everything works as expected.
2. Both _current_rendertarget and _current_depthstencil are null: Device wrapper has 3 references left when there are three references left in user code. Affected code path is thus entered too early, but since both pointers are null there is nothing to release. Everything works as expected.
3. Only one of _current_rendertarget and _current_depthstencil is not null: Device wrapper has 3 references left when there are two references left in user code. Affected code path is thus entered too early - non-null surface is released when releasing second to last pointer in user code. The last user pointer still holds a valid device handle, but its surfaces are already released! Device is left in semi-corrupted state!

Fixing this is trivial enough, albeit introduces even more thread unsafety to already thread unsafe device code (this should be considered a separate issue however). What do you think?

[crosire]

auto &rt = _current_rendertarget;
auto &ds = _current_depthstencil;
if (rt != nullptr && ds != nullptr && myRef <= 3)
{
    rt->Release(); rt = nullptr;
    ds->Release(); ds = nullptr;
}
else if (rt != nullptr && myRef <= 2)
{
    rt->Release(); rt = nullptr;
}
else if (ds != nullptr && myRef <= 2)
{
    ds->Release(); ds = nullptr;
}

[elishacloud]

I don't think those changes are safe. I tried that with one of my games (Rayman 3: Hoodlum Havoc) and it crashed on exit, whereas it never crashed before. Event Viewer showed it as crashing in d3d8.dll (this wrapper).

[crosire]

Mmh. Well, then it's probably better to follow the "Never change a running system" matra.

[elishacloud]

Ok, if I change the declaration then it works without crashing:

auto* rt = _current_rendertarget;
auto* ds = _current_depthstencil;
if (rt != nullptr && ds != nullptr && myRef <= 3)
{
    rt->Release(); rt = nullptr;
    ds->Release(); ds = nullptr;
}
else if (rt != nullptr && myRef <= 2)
{
    rt->Release(); rt = nullptr;
}
else if (ds != nullptr && myRef <= 2)
{
    ds->Release(); ds = nullptr;
}

[crosire]

That doesn't reset the fields to nullptr though, which is bad too.

[CookiePLMonster]

Indeed, auto& shouldn't be used in here. But other than this, it looks good and it does reset the fields to nullptr (missed the assignments after Release(), crosire?).

The only thing I'd change is <= to == - defensive approach is really not needed in here, I guess.

[elishacloud]

Yes, that code seems to work for me. No crashes or other issues.

[CookiePLMonster]

The one I posted in here was wrong again though (too sleepy, it seems). I'll fix and issue a PR.

[crosire]

No, it doesn't reset the fields to nullptr. It only sets the local auto variables to nullptr. But I see you fixed that in the PR.

[crosire]

I don't have any way to test #18 currently, so would greatly appreciate if @elishacloud could verify things still work with it.

[elishacloud]

Yes, I just verified #18 and it seemed to work fine. I did not find any issues with it.