Additional cleanup in oni_destroy_ctx

[jsiegle]

I needed to add these additional lines to avoid memory leaks on Windows with the acquisition board plugin. Let me know if this makes sense to merge in main.

[jonnew]

Although this commit generally speaks to architectural issues with the calling code, I think that there are some points in here worth thinking about. This library is not thread safe and I have suspicion that the issue here might be resulting from the calling code using different threads for context management a reading frames.

1. Replacing the actions proposed in this comment with debug assertions about non-zero ref counts at time of oni_destroy_ctx call. This seems obvious for guiding devleopment.

2. Freeing the dev_table does seem like a valid addition.

[jonnew]

This points to downstream code issues. The point of reference counting is that we are the oni_create_frame() or oni_read_frame() caller takes ownership/responsibility for a segment of memory that has been allocated by the library. These indicate that the memory is in use and should not be freed. Only the caller should be allowed to decrement the ref count, via matched call oni_destroy_frame().

It seems like there might be an ordering issue where oni_destroy_ctx() is being called before the last oni_destroy_frame() since doing a reference decrement (aka accounting for 1 missing frame) solves your issue. The issue with this is that the library is now ripping the rug out from some potentially other process and freeing memory it does not control, which will result in segfaults elsewhere.

[aacuevas]

I believe this fix might be valid.
Correct me if I'm missing something but the context does hold a reference to the shared buffer, as well as the frames. And I believe we are not releasing the reference hold by the context.

When a frame is created and a new shared buffer needs to be allocated through _oni_read_buffer or _oni_alloc_write_buffer

1. A new buffer is created with a reference count of 1
2. The old buffer reference count is decreased by 1 and the context pointer replaced, so only references on existing frames exist
3. The buffer is assigned to a new frame and its reference counter increased

When destroying the frame, the reference count for it is decreased, but the original 1 by the context reference is never decreased, even when destroying the context. So we need to release it from the context as well

[jonnew]

Ah, yes! that's true. OK so in fact, this solution is the appropriate fix indeed.

[jonnew]

This appears valid.

[jonnew]

@aacuevas I would like your opinion here as well.

[jonnew]

After the discussion with @aacuevas I realized my inital review as wrong. My only suggestion is to add an NB:

    // NB: _ref_dec is only called when a new shared buffer is created. We must
    // therefore decrement the active shared buffers here to balance the initial
    // _ref_inc from their creation.
    if (ctx->shared_rbuf != NULL)
        _ref_dec(&(ctx->shared_rbuf->count));

since this is a bit tricky.