does not happen in m149, but in m154 => regression

fs->cl,nn: seems there is a SvxShape which is not being notified that its
associated SdrObject was destructed. So, it accesses a dead pObj pointer, which
crashes.
Do you have any idea about this? Anything you changed in the drawing layer in
svx/sc between m149/m154, in particular with respect to UNO shapes?

This seem to be a threading problem.
- Thread A destroyes a ScShapeObj, which resets the mxUnoShape
  at the SdrObject.
- Thread B closes the document, and thus deletes all SdrObjects. Since
  mxUnoShape is already reset to NULL, the SdrObject cannot notify its SvxShape
  anymore.
- Thread B now proceedes to broadcasting a HINT_MODELCLEARED.
  The SvxShape then accesses a dead SdrObject.

Since this timing problem only indirectly depends on the nonpro/pro version of
OOo, but in theory also could happen in a pro build, I raise that target to 2.0.3.

Created attachment 34005 [details]
patch part 1 (svx)

Created attachment 34006 [details]
patch part 2 (sc)

fs->nn: I got a (preliminary, so far) approval from CL for part 1 of the patch.
Would you please have a look at part 2 and tell me any objections you might have?

It looks like the change in sc will lead to problems with Java components. When
a Java component is called from a user interaction in OOo, it runs in the main
thread, with the SolarMutex locked. So trying to acquire the SolarMutex in an
object's release method will block the finalizer thread as long as the original
call is running.

The ScShapeObj aggregates a SvxShapeControl. In ~ScShapeObj, the SvxShapeControl
is destroyed. It is derives (via SvxShapeTextand SvxUnoTextBase from
SvxUnoTextRangeBase, which in its destructor does a "delete pEditSource". This
pEditSource is a SvxTextEditSource instance, which in its destructor locks the
SolarMutex.

So, in short, when the ScShapeObj is destroyed, and aggregates a SvxShape which
contains text, the SolarMutex is locked, anyway, and thus the finalizer thread
is blocked.

The problem is that currently, the locking is *too late*. The shape instance is
in parts, but not completely, destroyed. The patch changes this to lock the
mutex earlier, to make the whole destruction a transaction.


However, I do not know whether there are other SvxShape instance which might not
indirectly lock the SolarMutex upon destruction. In this case, the patch might
produce additional problems. Could you elaborate where you expect them, so I can
check?

However, even then I think *not* locking the mutex is a bug, since none of all
those SvxShape implementations are really threadsafe. Only some methods rely on
the SolarMutex, some don't, and none has an own thread-safe concept with own
mutexes. So, in any case, I think destroying the shapes should be transactional,
and guarded by the SolarMutex.


Side note:
A less invasive patch is to change the ScShapeObj destructor as follows:
  ScUnoGuard aGuard;
  mxShapeAgg.clear();
That is, only guard the reset of the aggregated object (and thus its
destruction, since this is the only reference) with the SolarMutex. This also
solves the problem. However, I am not sure if this is better, since it loses the
transaction characteristics of the last release ...

Created attachment 34111 [details]
patch, Mk II

attached patch implements a more comprehensive fix, which only touches SVX, but
not SC (or the other applications).

Basic change compared to the previous patch is that now SvxShape::release is
overridden, making the "release the last reference, and delete |this|" an atomic
operation, guarded by the SolarMutex.

Additionally, methods of the SvxShape which could be called bypassing UNO (such
as the |dispose| call from SdrObject::~SdrObject, or SvxShape::Notify), but
create a reference on the SvxShape itself, have been protected against duplicate
deletion.
(That is, if thread A blocks in SvxShape::release, with the ref count already
dropped to 0, and thread B enters one of those methods, and creates a temporary
reference to the SvxShape instance, then this would result in duplicate "delete
this" calls, and thus a crash.)

This works with all integration tests from "forms", which make extensive use of
shapes in both Calc and Writer.
Not yet verified against various past bugs in this area ...

Fixed in CWS dba203a. CL Pointed out that it's possible to hold a weak reference
to an SdrObject. So now the SvxShape does exactly this, and thus evades the
problem of accessing a pointer to an already-deleted object.
This solution is far more elegant that what I originally suggested ...

fs-> msc: please verify in CWS dba203a

re-open issue and reassign to msc

reassign to msc

reset resolution to FIXED

verified in cws dba203a

Hi,

this is fixed in the current master. The current master is available at
http://download.openoffice.org/680/index.html

I close this issue now.

Bye Marc