bad API design

[John Ralls]

On Oct 17, 2011, at 11:34 AM, Christian Stimming wrote:

> Am Montag, 17. Oktober 2011 schrieb John Ralls:
>> On Sep 26, 2011, at 11:35 AM, Christian Stimming wrote:
>>> Am Montag, 26. September 2011 schrieb John Ralls:
>>>> I have a plan to rework the engine (the module which does all of
>>>> the accounting and business calculations) in stages. Stage one is to
>>>> write comprehensive tests for each class. Stage two (which will
>>>> interleave with stage one) is to overhaul each class to be a
>>>> well-behaved GObject class with all access to data (including KVP) via
>>>> function calls instead of passing out pointers for other objects to
>>>> modify at will.
>>> 
>>> Out of curiosity: Which examples for "passing out pointers" do you have
>>> in mind here? Surely there is plenty of access to KVP data by simply
>>> accessing the KVP directly, which is bad, but is there also direct
>>> pointer manipulation in that many places?
>> 
>> Here's an example I found today, while investigating bug 661903:
>> xaccTransGetSplitList() returns a GList* of the splits. The requestor can
>> manipulate or delete the splits, unbalancing or even eviscerating the
>> transaction without the transaction knowing. It's called in 23 places in C
>> and 19 in Scheme.
>> 
>> It doesn't help that Accounts also keep a list of splits and since we're
>> not using GObject reference counting, there's no way to protect against
>> one or both from having invalid pointers.
> 
> Absolutely. All of the API functions passing out a GList* are prone to this 
> error. However, that's an inherent issue of any C API, because the only 
> alternative is to pass out a newly allocated GList* each time. You can easily 
> guess this will lead to plenty of memory leaks... well... thinking about it 
> again, this might not be as bad as it sounds. Maybe those functions should 
> indeed pass out a newly allocated GList instead of the internal one, i.e. just 
> using g_list_copy. Then again, the data pointers will still be plain non-const 
> pointers as there isn't a const variant of GList*...

That's only part of the problem. The bug (and it's 661093, not 661903, sorry) is reporting a crash caused by trying to access a split that's been freed. We have several objects holding references to splits (transactions, accounts, lots, and business entities come immediately to mind); in addition, register windows need to have active access to split lists in order to edit them. Aside from memory management, problems arise in notifying all of the holders of references to a split that the split isn't valid. I think that's supposed to be done with QofSignals and handlers, but there's at least one demonstrated hole in the process.

There are a couple of approaches to handling all of the references, but I don't yet know which one will best enforce database integrity. They all involve getting everything into a clean GObject first so that reference counting and weak references can be used to manage object lifetime, so we can defer worrying about it for a while.

Regards,
John Ralls