Re: [Libreoffice] cppu::OPropertySetHelper ABI backwards compatibility

On Sun, Aug 21, 2011 at 12:08:23PM +0200, Lionel Elie Mamane wrote:
> On Sun, Aug 21, 2011 at 01:54:17AM +0200, Lionel Elie Mamane wrote:

> > 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.

> Maybe not. It seems my build was hosed, possibly because of wrong
> inter-module dependencies so that a change to
> cppuhelper/propshlp.hxx is not properly propagated everywhere.

> I'm making new tests doing "make clean && make" every time, and I'll
> let you know what I find out.

I confirm that ABI is broken for cppu::OPropertySetHelper with respect
to 3.4 branch.

I observe, additionally to the "wrong function called from new ABI
code to old ABI code that derives (inherits) from
cppu::OPropertySetHelper" that I already mentioned, that the
destructor for cppu::OPropertySetHelper::Impl is called with
non-sensical "this" pointer when (in old ABI code) a reference to a
class that inherits from cppu::OPropertySetHelper goes out of scope
and the object is destroyed. I have the case with postgresql-sdbc
compiled against libreoffice-3-4.

Here's an example trace of cppu::OPropertySetHelper::Impl ctor/dtor
calls:

Impl ctor this='0x2e851a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e97418'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e862e8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e97598'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e8d0a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e89128'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91638'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91fb8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e92288'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e92558'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e93238'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91518'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e94298'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e94ac8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e95148'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e95248'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9a0a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9bc48'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8ebd8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8d098'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8db48'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f95438'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f93ec8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f98118'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f96a98'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9ade8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f99778'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9db28'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9c458'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa0108'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9a318'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa37f8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa21d8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa6568'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa4e98'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fae6f8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fc7978'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fc8398'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fbefa8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl dtor this='0x7ffdb09d57c8'; m_handles.size()='148277658', 
m_newValues.size()='6148914691236407954', m_oldValues.size()='-6148914691211804262'


The attached patch makes both these symptoms go away, which is why I
conclude that ABI has changed; I'm not saying this patch should be
applied as-is; rather it should go this way, as mentioned in my first
email:

class OPropertySetHelperFireEventOption : public OPropertySetHelper,
                                          public ::com::sun::star::beans::XPropertySetOptions
{
    bool m_bFireEvent;
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.

If people agree that this would be the way to go, I'll prepare a patch
that does that; I'm also looking for a better class name than
OPropertySetHelperFireEventOption :)

-- 
Lionel

diff --git a/cppuhelper/inc/cppuhelper/propshlp.hxx b/cppuhelper/inc/cppuhelper/propshlp.hxx
index 670ce03..6b4f33e 100644
--- a/cppuhelper/inc/cppuhelper/propshlp.hxx
+++ b/cppuhelper/inc/cppuhelper/propshlp.hxx
@@ -351,8 +351,7 @@ public:
  */
 class OPropertySetHelper : public ::com::sun::star::beans::XMultiPropertySet,
                            public ::com::sun::star::beans::XFastPropertySet,
-                           public ::com::sun::star::beans::XPropertySet,
-                           public ::com::sun::star::beans::XPropertySetOption
+                           public ::com::sun::star::beans::XPropertySet
 {
 public:
     /**
@@ -505,11 +504,6 @@ public:
         const ::com::sun::star::uno::Sequence< ::rtl::OUString >& PropertyNames,
         const ::com::sun::star::uno::Reference< ::com::sun::star::beans::XPropertiesChangeListener 
> & Listener )
         throw(::com::sun::star::uno::RuntimeException);
-
-    // XPropertySetOption
-    virtual void SAL_CALL enableChangeListenerNotification( sal_Bool bEnable )
-        throw(::com::sun::star::uno::RuntimeException);
-
     /**
        The property sequence is created in the call. The interface isn't used after the call.
      */
@@ -637,8 +631,6 @@ protected:
      */
     OMultiTypeInterfaceContainerHelperInt32 aVetoableLC;
 
-    bool m_bFireEvent;
-
     class Impl;
 
     /** reserved for future use. finally, the future has arrived...
diff --git a/cppuhelper/source/propshlp.cxx b/cppuhelper/source/propshlp.cxx
index 7878062..4f19ba2 100644
--- a/cppuhelper/source/propshlp.cxx
+++ b/cppuhelper/source/propshlp.cxx
@@ -173,7 +173,6 @@ OPropertySetHelper::OPropertySetHelper(
     : rBHelper( rBHelper_ ),
       aBoundLC( rBHelper_.rMutex ),
       aVetoableLC( rBHelper_.rMutex ),
-      m_bFireEvent(true),
       m_pReserved( new Impl(false, 0) )
 {
 }
@@ -183,7 +182,6 @@ OPropertySetHelper::OPropertySetHelper(
     : rBHelper( rBHelper_ ),
       aBoundLC( rBHelper_.rMutex ),
       aVetoableLC( rBHelper_.rMutex ),
-      m_bFireEvent(true),
       m_pReserved( new Impl( bIgnoreRuntimeExceptionsWhileFiring, 0 ) )
 {
 }
@@ -194,7 +192,6 @@ OPropertySetHelper::OPropertySetHelper(
     : rBHelper( rBHelper_ ),
       aBoundLC( rBHelper_.rMutex ),
       aVetoableLC( rBHelper_.rMutex ),
-      m_bFireEvent(true),
       m_pReserved(
         new Impl( bIgnoreRuntimeExceptionsWhileFiring, i_pFireEvents) )
 {
@@ -218,7 +215,6 @@ Any OPropertySetHelper::queryInterface( const ::com::sun::star::uno::Type & rTyp
     return ::cppu::queryInterface(
         rType,
         static_cast< XPropertySet * >( this ),
-        static_cast< XPropertySetOption * >( this ),
         static_cast< XMultiPropertySet * >( this ),
         static_cast< XFastPropertySet * >( this ) );
 }
@@ -631,9 +627,6 @@ void OPropertySetHelper::fire
     sal_Bool bVetoable
 )
 {
-    if (!m_bFireEvent)
-        return;
-
     OSL_ENSURE( m_pReserved.get(), "No OPropertySetHelper::Impl" );
     if (m_pReserved->m_pFireEvents) {
         m_pReserved->m_pFireEvents->fireEvents(
@@ -1037,12 +1030,6 @@ void OPropertySetHelper::firePropertiesChangeEvent(
     delete [] pHandles;
 }
 
-void OPropertySetHelper::enableChangeListenerNotification( sal_Bool bEnable )
-    throw(::com::sun::star::uno::RuntimeException)
-{
-    m_bFireEvent = bEnable;
-}
-
 #ifdef xdvnsdfln
 // XPropertyState
 PropertyState OPropertySetHelper::getPropertyState( const OUString& PropertyName )