Please clarify when cd_icc_load_handle() transfers ownership

[smcv]

cd_icc_load_handle() is documented as taking ownership of the lcms profile object, similar to g_value_take_boxed(). That's probably the most sensible choice for an object that isn't ref-counted. However, unlike most "take" functions, cd_icc_load_handle() can fail, which makes its semantics much less obvious.

At the moment, there are two failing code paths: on success it takes ownership, on failure to parse the profile it also takes ownership, but if the profile was created with an inappropriate (non-thread-safe) lcms context, it does not take ownership.

mutter currently behaves as though only success takes ownership, but I think that's a bug: https://gitlab.gnome.org/GNOME/mutter/-/issues/2659

Unfortunately, I think it would be an incompatible ABI change for libcolord to change this function to always take ownership, because that would turn a previously-valid use into a double-free. Similarly, changing it to only take ownership on success (even if possible) would turn a previously-valid use into a memory leak, which is less bad but also not great.

The best way might be to deprecate the function, and have a replacement with easier-to-document semantics?

[hughsie]

but if the profile was created with an inappropriate (non-thread-safe) lcms context, it does not take ownership

I think using the non-thread-safe lcms context is the very unusual case; it's also a programmer error IIRC. Maybe doing something like g_return_val_if_fail(cmsGetProfileContextID (handle) != NULL, FALSE) would be more appropriate?

[hughsie]

...or move the priv->lcms_profile = handle above the check for the context?

[smcv]

I think using the non-thread-safe lcms context is the very unusual case; it's also a programmer error IIRC. Maybe doing something like g_return_val_if_fail(cmsGetProfileContextID (handle) != NULL, FALSE) would be more appropriate?

Yes, I was thinking along similar lines. That would simplify the semantics to: programmer error has undefined behaviour; otherwise the function always takes ownership, regardless of success or error.

(In this case the UB would take the form of the lcms handle being leaked, although as with any UB, exactly what will happen isn't an API guarantee.)

...or move the priv->lcms_profile = handle above the check for the context?

If it isn't already considered to be programmer error, then my concern is that starting to transfer ownership, in a situation where previously it wasn't transferred, could make some previously working code start to double-free.

[hughsie]

Yes, I was thinking along similar lines.

If you do a PR request, I'd merge it :)