[Libreoffice] cppu::OPropertySetHelper ABI backwards compatibility

Hi,

I'm writing about your commits

11207ae93191fb966676423e6d377c8292a8cf0b
    Make XPropertSet2 not a child interface of XPropertySet.

    This is to preserve ABI backward compatibility with
    cppu::OPropertySetHelper.

80b1e662777100a7dfd80176a2b528880a838167

    Added XPropertySet2 to allow disabling of change event
    notifications.


From the second (later) commit log message, you intended to preserve
ABI, but I get the impressions ABI backwards compatibility is still
broken in a different way by the addition of

    virtual void SAL_CALL enableChangeListenerNotification( sal_Bool bEnable )
        throw(::com::sun::star::uno::RuntimeException);

From what I observe, at least with GNU GCC/g++ on Debian GNU/Linux
amd64 (I wouldn't know about MSVC++ and g++ on MS Windows, but
possibly it is the same), it seems all the virtual functions that are
declared *after* that one in the header are shifted one position in
the virtual function table, and that lookups in the virtual function
table are by position, not by name/signature.

So if a class is compiled against the *old* ABI (e.g. an extension
compiled against libreoffice-3-4), but a method (declared after your
addition in the header) is called from code compiled with the *new*
ABI (e.g. LibreOffice master / 3.5), then the wrong virtual function
gets called: the one *after* the one that is meant is called.

For example, it makes LibreOffice master segfault when using a
postgresql-sdbc compiled against LibreOffice 3.4.x when opening a
connection, because

dbaccess::OConnection::getTables
calls (indirectly)
dbaccess::OFilteredContainer::construct
calls
pq_sdbc_driver::DatabaseMetaData::getTables
calls (indirectly)
pq_sdbc_driver::PreparedStatement::executeQuery
calls (indirectly)
cppu::OPropertySetHelper::getPropertyValue
which calls the virtual function
 getInfoHelper();
on a pq_sdbc_driver::PreparedStatement
which derives from cppu::OPropertySetHelper.

But the function that is actually called is the *next* one in the (old
ABI) virtual function table (because of the shift mentioned above),
namely convertFastPropertyValue... the arguments it gets are
nonsensical (in particular, when I run it, a negative integer used as
index into an array -> segfault).


Moving enableChangeListenerNotification( sal_Bool bEnable ) to the end
of OPropertySetHelper would (I expect) fix that particular problem,
but I expect that it will lead to problems when
enableChangeListenerNotification is called, because old ABI derived
classes will not have that entry in their virtual function table.


So I'm not sure what to do... Maybe put OPropertySetHelper back like
it was, and have:


class OPropertySetHelper2 : public OPropertySetHelper
{
public:
(...)
    virtual void SAL_CALL enableChangeListenerNotification( sal_Bool bEnable )
        throw(::com::sun::star::uno::RuntimeException);
}

So that new code that wants to have the ability to enable/disable
change listener notifications can derive from OPropertySetHelper2
instead, but old code still works?

-- 
Lionel