cef_client getters should be idempotent?

[aleitner]

I'm a little concerned about these getters now having side effects (one-time init is performed as a part of retrieval). Is the getter known to only ever be invoked once or if more than once is the struct freed before the getter is called again? I tried to make the getter idempotent, returning the same struct that was already initialized, but it seems like the reference counting is causing a double free error with this change.

Here is the only documentation I have seen for the getters https://github.com/chromiumembedded/cef ... ent_capi.h

Below is my code if the getter is expected to only be called once, or if the struct that the getter creates is freed before being called again.
Request Handler Getter

Code: Select all

static cef_request_handler_t* get_request_handler(cef_client_t* self) {
    http_cef_request_handler_t* request_handler = http_cef_request_handler_init();

    return &request_handler->base;


Initialize the client and set the request handler getter

Code: Select all

http_cef_client_t* http_cef_client_init() {
    http_cef_client_t* custom_client = (http_cef_client_t *)base_calloc(1,
            sizeof(http_cef_client_t));

    /* Initialize base structure */
    cef_client_t* client = &custom_client->base;
    client->base.size = sizeof(cef_client_t);
    client->base.add_ref = add_ref_generic;
    client->base.release = release_generic;

    client->get_request_handler = get_request_handler;

    return custom_client;
}


Request Handler Initializer

Code: Select all

http_cef_request_handler_t* http_cef_request_handler_init() {

    /* Allocate memory for the custom request handler */
    http_cef_request_handler_t* handler =
        (http_cef_request_handler_t *)base_calloc(1, sizeof(http_cef_request_handler_t));

    /* Initialize base structure with its size and define callbacks */
    handler->base.base.size = sizeof(cef_request_handler_t);
    handler->base.base.add_ref = add_ref_generic;
    handler->base.base.release = release_generic;

    /* Return the new request handler instance */
    return handler;
}


[magreenblatt]

Is the getter known to only ever be invoked once or if more than once is the struct freed before the getter is called again?

Getters may be invoked multiple times, unless otherwise documented. You should provide a correct ref-count implementation in all cases.

[aleitner]

This leads me to wonder 2 possible things:
1) The current implementation is correct as reference counting is set up for the handlers and that any structs created will be freed through reference counting.
or
2) We need to manually increase the reference count every time the getter is called

[aleitner]

Actually after some testing I manually increased the reference count but we continued to get a double free issue so it looks like my initial implementation is correct provided the reference counting is as intended?

Code: Select all

static cef_request_handler_t* get_request_handler(cef_client_t* self) {
    http_cef_client_t* custom_client = (http_cef_client_t*)self;
   
    if (custom_client->request_handler == NULL) {
        custom_client->request_handler = http_cef_request_handler_init();
    }

    else {
        custom_client->request_handler->base.base.add_ref(&custom_client->request_handler->base.base);
    }

    return &custom_client->request_handler->base;
}


[magreenblatt]

so it looks like my initial implementation is correct provided the reference counting is as intended?

You haven’t shown your refcount implementation (add_ref_generic, release_generic, etc) so I can’t comment on whether it’s correct. I suggest you read https://bitbucket.org/chromiumembedded/ ... TheCAPI.md

[aleitner]

Here is our implementation of reference counting.

Code: Select all

typedef struct _ref_counter_block {
    /**
     *  The mutex ensuring thread-safety when changing the reference count.
     */
    pthread_mutex_t mutex; 

    /**
     * The reference counter which records the number of references to a specific struct.
     */
    int ref_count;
} ref_counter_block;

void* base_calloc(size_t num, size_t size) {
    /* New size including extra space for atomic_int. */
    size_t new_size = num*size + sizeof(ref_counter_block);
    ref_counter_block* block = (ref_counter_block*) calloc(1, new_size);
   
    /* Initialize the reference count. */
    pthread_mutex_init(&block->mutex, NULL);
    block->ref_count = 1;

    /* Return pointer to the block after the atomic_int. */
    return (void*)(block + 1);
}

void base_free(void* ptr) {
    /* Adjust the pointer to the beginning of the actual allocated block, then free it */
    ref_counter_block* block = (ref_counter_block*)ptr - 1;
    pthread_mutex_destroy(&block->mutex);
    free(block);
}

void add_ref_generic(cef_base_ref_counted_t* base) {
    /* Ensure base is not NULL */
    if (base == NULL)
        return;
   
    ref_counter_block* block = (ref_counter_block*)base - 1;
   
    pthread_mutex_lock(&block->mutex);
    block->ref_count++;
    pthread_mutex_unlock(&block->mutex);
}

int release_generic(cef_base_ref_counted_t* base) {
    /* Ensure base is not NULL */
    if (base == NULL)
        return 1;

    ref_counter_block* block = (ref_counter_block*)base - 1;
    pthread_mutex_lock(&block->mutex);
    int last_ref = --block->ref_count == 0;
    pthread_mutex_unlock(&block->mutex);

    if (last_ref) {
        base_free(base);
        return 1;
    }
   
    return 0;
}


[magreenblatt]

/* Return pointer to the block after the atomic_int. */
return (void*)(block + 1);

This logic adding 1 doesn’t look correct. Shouldn’t that be “sizeof(ref_counter_block)”? Same below where you subtract 1.

[aleitner]

In the provided code, the custom allocation function base_calloc() is allocating extra space for a ref_counter_block alongside each object. This ref_counter_block holds the mutex for thread-safety and the actual reference count.

In this way, the reference counting mechanism is fully encapsulated within the object's memory block, effectively "hidden" from the outward interface. From the outside, you only interact with the object itself, with no direct access to or awareness of the ref_counter_block.

Here's the base_calloc() function to illustrate this:

Code: Select all

void* base_calloc(size_t num, size_t size) { /* New size including extra space for ref_counter_block. / size_t new_size = numsize + sizeof(ref_counter_block); ref_counter_block* block = (ref_counter_block*) calloc(1, new_size);

/* Initialize the reference count. */
pthread_mutex_init(&block->mutex, NULL);
block->ref_count = 1;

/* Return pointer to the block after the ref_counter_block. */
return (void*)(block + 1);
}


In this function, a block of memory is allocated with enough space for the desired object AND the ref_counter_block. The mutex and reference count are initialized, and then a pointer to the memory immediately following the ref_counter_block (which is where the requested object space starts) is returned.

This arrangement makes the ref_counter_block completely invisible during normal usage of the object, but conveniently accessible when we need to adjust the reference count, without any need for additional memory allocations or complex data structures to track the counts. It provides an efficient and elegant solution for implementing reference counting mechanism in C.

As you probably know, pointer arithmetic on a pointer to a data structure treats increment or decrement operations as steps in units of the structure's entirety, not as byte steps.

So when doing:

Code: Select all

 return (void*)(block + 1);


This is actually adding sizeof(ref_counter_block) to the pointer block under the hood because block is a ref_counter_block* type. It moves the pointer forward by one ref_counter_block in memory. The +1 here is not adding one byte, but one ref_counter_block.

[magreenblatt]

This is actually adding sizeof(ref_counter_block) to the pointer block under the hood because block is a ref_counter_block* type. It moves the pointer forward by one ref_counter_block in memory. The +1 here is not adding one byte, but one ref_counter_block.

Right, my mistake.

[aleitner]

So why would returning a single instance of an object, with a correct reference count, result in a double-free?

I want to know what could be happening in cef to explain why my getters always returning new reference counted objects is the correct way to do it.

CEF anticipates that getter functions might be called multiple times. Each time a getter function is invoked, it retrieves a wholly new instance of an object and increments our reference count.

On the other hand, if we decide to have getters return pre-existing instances, every caller would get references to this same object whenever they call our getter functions. What I think is happening is when each caller is done using this object, they would try to decrement the reference count of the object. The count would eventually drop to zero and the object would get freed. However, if another call to the getter occurs and returns this already-freed object, it would lead to a double-free error when the other calls to the getter would try to decrement the reference.

Considering the behavior of CEF and the detail of our implementation that we have uncovered, it appears the correct approach is for getters to always return new reference counted objects. This ensures that every caller fetches a new, dedicated instance of the struct, and it can safely decrease the reference count and even free it without interfering with other threads within CEF. It seems the design of our getters is to support this safe, independent use of resources, rather than to expect all callers to share the resources in a coordinated way.