Issue 61771 - crash when executing forms.integration.CellBinding test case (non-pro only)
Summary: crash when executing forms.integration.CellBinding test case (non-pro only)
Alias: None
Product: gsl
Classification: Code
Component: code (show other issues)
Version: 680m154
Hardware: All All
: P2 Trivial (vote)
Target Milestone: OOo 2.0.3
Assignee: marc.neumann
QA Contact: issues@gsl
Keywords: regression
Depends on:
Reported: 2006-02-08 09:16 UTC by Frank Schönheit
Modified: 2006-08-29 08:54 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation in: ---
Developer Difficulty: ---

patch part 1 (svx) (8.13 KB, patch)
2006-02-09 14:52 UTC, Frank Schönheit
no flags Details | Diff
patch part 2 (sc) (1.18 KB, patch)
2006-02-09 14:53 UTC, Frank Schönheit
no flags Details | Diff
patch, Mk II (12.17 KB, patch)
2006-02-13 11:00 UTC, Frank Schönheit
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Frank Schönheit 2006-02-08 09:16:38 UTC
- execute the test case "intgration.forms.CellBinding"
  (do a dmake run_cellbinding in forms/qa/integration/forms)
=> upon closing the test document, OOo crashs

Only the non-product version seems to be affected ...
Comment 1 Frank Schönheit 2006-02-08 09:18:53 UTC
does not happen in m149, but in m154 => regression
Comment 2 Frank Schönheit 2006-02-08 09:21:39 UTC
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
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?
Comment 3 Frank Schönheit 2006-02-08 11:37:12 UTC
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
- 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.
Comment 4 Frank Schönheit 2006-02-09 14:52:38 UTC
Created attachment 34005 [details]
patch part 1 (svx)
Comment 5 Frank Schönheit 2006-02-09 14:53:25 UTC
Created attachment 34006 [details]
patch part 2 (sc)
Comment 6 Frank Schönheit 2006-02-09 15:44:22 UTC
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?
Comment 7 niklas.nebel 2006-02-09 17:31:15 UTC
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.
Comment 8 Frank Schönheit 2006-02-10 06:53:33 UTC
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

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

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;
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 ...
Comment 9 Frank Schönheit 2006-02-13 11:00:47 UTC
Created attachment 34111 [details]
patch, Mk II
Comment 10 Frank Schönheit 2006-02-13 11:06:21 UTC
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
(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 ...
Comment 11 Frank Schönheit 2006-03-08 09:43:34 UTC
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 ...
Comment 12 Frank Schönheit 2006-03-22 06:05:44 UTC
fs-> msc: please verify in CWS dba203a

re-open issue and reassign to msc
Comment 13 Frank Schönheit 2006-03-22 06:05:49 UTC
reassign to msc
Comment 14 Frank Schönheit 2006-03-22 06:05:53 UTC
reset resolution to FIXED
Comment 15 marc.neumann 2006-03-27 11:12:46 UTC
verified in cws dba203a
Comment 16 marc.neumann 2006-08-29 08:54:34 UTC

this is fixed in the current master. The current master is available at

I close this issue now.

Bye Marc