Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

out-structs containing objects and refcounting #326

Closed
Kangz opened this issue Sep 4, 2024 · 3 comments
Closed

out-structs containing objects and refcounting #326

Kangz opened this issue Sep 4, 2024 · 3 comments
Labels
has resolution Issue is resolved, just needs to be done

Comments

@Kangz
Copy link
Collaborator

Kangz commented Sep 4, 2024

We should define what happens to objects contained in out-structs that will be overwritten. I had the following code and it was leaking quite unexpectedly:

WGPUSurfaceTexture s;

wgpuSurfaceGetCurrentTexture(&s);
wgpuSurfacePresent();
wgpuSurfaceGetCurrentTexture(&s);

wgpuSurfaceTextureFreeMembers(&s);

The issue is that the second wgpuSurfaceGetCurrentTexture call overwrites s.texture without decrementing the refcount, which means that wgpuSurfaceTextureFreeMembers doesn't know to unref it. (or maybe it is a C++ism of Dawn that we need to fix, but in all cases the behavior is surprising).

In all cases I'd suggest that if an object is passed in an out-struct, it must be either null or valid, and if valid it will be released once.

@kainino0x
Copy link
Collaborator

wgpuSurfaceTextureFreeMembers doesn't exist (I got confused about this at some point) because there are no arrays of data to free.

Generally speaking, just because there's a pointer value for a WGPUTexture in WGPUSurfaceTexture doesn't mean that's a valid reference. It could have been Released already and the app didn't zero out the pointer because they knew they were about to overwrite it:

wgpuTextureRelease(s.texture);
wgpuSurfaceGetCurrentTexture(&s); // double free

I think it's the reality of the C API that we don't know what the intent was so we should just overwrite the pointer without releasing it. It's unfortunate that the overwrite is hard to see (vs. if we just did texture = wgpuSurfaceGetCurrentTexture(); which is more obviously a leak) but I think it's fine.

In our C++ API, we do know whether the ref is still there because the RAII wrapper already needs to know. If we implemented the C++ API in C++ that would happen automatically, because we'd write s->texture = nextTexture; and that would release the existing ref. I think it's just because of our C/C++ reinterpret_casting that we don't currently do that, so we would need to do s->texture = {}; before calling from the C++ wrapper into the C API.

@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Sep 7, 2024
@kainino0x
Copy link
Collaborator

Sep 12 meeting:

  • (brief discussion, no really strong opinions)
  • Sounds good to leak the ref in this case (rather than releasing)

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. and removed !discuss Needs discussion (at meeting or online) labels Sep 16, 2024
@kainino0x
Copy link
Collaborator

Closed, still needs docs.

@kainino0x kainino0x removed the needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has resolution Issue is resolved, just needs to be done
Projects
None yet
Development

No branches or pull requests

2 participants