AccessViolationException caused by garbage collection of managed wrappers

[ChernyshevDS]

Hello. I'm facing a problem with Xbim: when loading certain models I get AccessViolationException somewhere inside geometry core of Xbim (stacktrace is in the end of this issue). The issue repeats on different models irregularly - a model may load successfully 9 times of 10.

Here I'll provide some investigations.

Stacktrace mentions Tasks.Parallel, so my first guess was the concurrency issues. Code I'm using to open IFC models looks like this:

using (var model = IfcStore.Open(filePath))
{
    var context = new Xbim3DModelContext(model)
    {
        MaxThreads = maxThreads,
    };
    context.CreateContext(progressDelegate, adjustWcs: true);
    ...
}

The crashed method is CreateContext. Xbim3DModelContext allows to set maximum number of processing threads, and indeed, when setting MaxThreads to 1 the issue does not show up. Unfortunately, it's not an option, because processing time becomes too large.

I'm loading models which contain a lot of reinforcing bars. In IFC these bars are defined as a circle swept along a curve, so the most used method is XbimSolid::Init(IIfcSweptDiskSolid^ repItem, ILogger^ logger).

Here is the code to demonstrate my point, stripped of any irrelevant details:

void XbimSolid::Init(IIfcSweptDiskSolid^ repItem, ILogger^ logger)
{
    XbimWire^ sweep = CreateDirectrix(repItem->Directrix, repItem->StartParam, repItem->EndParam, logger);
    if (!sweep->IsValid)
        return;
    ... some code using sweep variable ...
    String^ err = BuildSweptDiskSolid(sweep, ...);
    ... more code that doesn't use sweep variable ...
}

String^ XbimSolid::BuildSweptDiskSolid(const TopoDS_Wire& directrixWire, double radius, ...)
{
    ...
}

The exception is raised inside BuildSweptDiskSolid, because directrixWire is not initialized. But it was verified right after CreateDirectrix (IsValid checks entity state)!

I've noticed, that XbimWire is not a subclass of TopoDS_Wire, that's required by BuildSweptDiskSolid - this code compiles because XbimWire has an implicit conversion operator:

ref class XbimWire : IXbimWire, XbimOccShape
{
private:
    IntPtr ptrContainer;
    virtual property TopoDS_Wire* pWire
    {
        TopoDS_Wire* get() sealed { return (TopoDS_Wire*)ptrContainer.ToPointer(); }
        void set(TopoDS_Wire* val)sealed { ptrContainer = IntPtr(val); }
    }

public:
    operator const TopoDS_Wire& () { return *pWire; }
}

Please notice that returned TopoDS_Wire instance is created in unmanaged memory and stored in IntPtr field, so it can not be tracked by GC.

My point is: XbimWire instance gets destroyed by garbage collector right after operator TopoDS_Wire execution, destroying wrapped native object referenced by ptrContainer, causing null pointer dereference in native code. XbimWire object in not referenced anywhere after operator call in XbimSolid::Init, so GC can rightfully destroy it.

It can be fixed by adding GC.KeepAlive(sweep); at the end of XbimSolid::Init. I've tried it, and indeed the issue was gone, but only in this method: Xbim crashed in XbimEdge::Init(IIfcCurve^ curve, ILogger^ logger) 🤦.

Conclusion

I'm not sure, why this issue has appeared just now. MS had an analyzer warning regarding usage of unmanaged resources in managed code. As far as I can see, there are another places like this in Xbim codebase, where GC can reclaim managed memory destroying native resources underneath, while they are still in use. Maybe it's just I'm tryin to load rather large models in limited environment that causes GC to act a lot more often, causing issues like this one.

Thank you for your attention, I hope I can get some insight from devs.

Assemblies and versions affected:

I'm using XbimGeometry 5.1.403.

Additional Details

Crash stacktrace:

Unhandled exception: System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
at Xbim.Geometry.XbimSolid.BuildSweptDiskSolid(TopoDS_Wire* directrixWire, Double radius, Double innerRadius, BRepBuilderAPI_TransitionMode transitionMode)
   at Xbim.Geometry.XbimSolid.Init(IIfcSweptDiskSolid solid, ILogger logger)
   at Xbim.Geometry.XbimSolid..ctor(IIfcSweptDiskSolid solid, ILogger logger)
   at Xbim.Geometry.XbimGeometryCreator.CreateSolid(IIfcSweptDiskSolid ifcSolid, ILogger logger)
   at Xbim.Geometry.XbimGeometryCreator.Create(IIfcGeometricRepresentationItem geomRep, IIfcAxis2Placement3D objectLocation, ILogger logger)
   at Xbim.Geometry.Engine.Interop.XbimGeometryEngine.Create(IIfcGeometricRepresentationItem ifcRepresentation, ILogger logger) в D:\Projects\Repos\XbimGeometry\Xbim.Geometry.Engine.Interop\XbimGeometryEngine.cs:строка 74
   at Xbim.ModelGeometry.Scene.Xbim3DModelContext.<>c__DisplayClass39_0.<WriteShapeGeometries>b__0(Int32 shapeId) в C:\bu   в System.Threading.Tasks.Parallel.<>c__DisplayClass42_0`2.<PartitionerForEachWorker>b__1()
   at System.Threading.Tasks.Task.<>c__DisplayClass176_0.<ExecuteSelfReplicating>b__0(Object <p0>)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
   at System.Threading.Tasks.Task.ExecuteEntry(Boolean bPreventDoubleExecution)
   at System.Threading.Tasks.ThreadPoolTaskScheduler.TryExecuteTaskInline(Task task, Boolean taskWasPreviouslyQueued)
   at System.Threading.Tasks.TaskScheduler.TryRunInline(Task task, Boolean taskWasPreviouslyQueued)
   at System.Threading.Tasks.Task.InternalRunSynchronously(TaskScheduler scheduler, Boolean waitForCompletion)
   at System.Threading.Tasks.Parallel.PartitionerForEachWorker[TSource,TLocal](Partitioner`1 source, ParallelOptions parallelOptions, Action`1 simpleBody, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
   at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
   at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
   at Xbim.ModelGeometry.Scene.Xbim3DModelContext.WriteShapeGeometries(XbimCreateContextHelper contextHelper, ReportProgressDelegate progDelegate, IGeometryStoreInitialiser geometryStore, XbimGeometryType geomStorageType)
   at Xbim.ModelGeometry.Scene.Xbim3DModelContext.CreateContext(ReportProgressDelegate progDelegate, Boolean adjustWcs) в C:\buildagent_os\_work\1\s\Xbim.ModelGeometry.Scene\Xbim3DModelContext.cs:строка 721
   at MRS.XbimCreator.XbimModel.StressTest(String filePath, Int32 maxThreads, IProgress`1 progress, ILoggerFactory loggerFactory)
   at MRS.XbimConverter.Program.Convert(String[] args)
   at MRS.XbimConverter.Program.Main(String[] args)

[CBenghi]

Thanks @ChernyshevDS,
I'll be looking into geometry in the coming days as a few other issues have surfaced.
Feel free to get in touch with a PR if you find a solution.

Can you clarify what you mean with the following?

but only in this method: Xbim crashed in XbimEdge::Init(IIfcCurve^ curve, ILogger^ logger) 🤦.

I think you say that the issue is gone in some place but resurfaces in the init of XbimEdge?
Thnaks,
Claudio

[ChernyshevDS]

Thanks for the reply!

I think you say that the issue is gone in some place but resurfaces in the init of XbimEdge?

Yes. I've added GC::KeepAlive at the end of the XbimSolid::Init method, and I haven't got any exceptions in this method. But after running several tests converting the same problematic IFC model I've got another AccessViolation, in different place (XbimEdge::Init), but for the same exact reason. So I've added GC::KeepAlive in this method too, and I think it resolved the issue for me, because I can not reproduce it anymore.

So my only concern is that GC::KeepAlive may be easily missed in some other places around Xbim codebase. This issue may arise in specific conditions, when memory consumption is high and GC is very active, so it can be hard to reproduce.

By the way, I could not reproduce the issue when building the Xbim.Geometry binary in Debug mode. I guess that is because compiler optimizations are disabled in Debug, and somehow GC is not so aggressive when reclaiming managed memory.

[CBenghi]

That's great.
Would you mind preparing a PR for this?

So my only concern is that GC::KeepAlive may be easily missed in some other places around Xbim codebase. This issue may arise in specific conditions, when memory consumption is high and GC is very active, so it can be hard to reproduce.

Sure that's a possibility, it does depend on the aggressiveness of GC.
In production we generally convert the model in batches to reduce this risk, but we are also keen of removing places where this might be a problem

Thanks.

[ChernyshevDS]

Would you mind preparing a PR for this?

Sure, I can try. Which branch should I target for? development branch is... in development state.

[CBenghi]

Yes, development would be good.

…

On Tue, 1 Mar 2022 at 14:58, ChernyshevDS ***@***.***> wrote: Would you mind preparing a PR for this? Sure, I can try. Which branch should I target for? development branch is... in development state. — Reply to this email directly, view it on GitHub <#370 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJY7MMKZDEJRPK4ID6IVFLU5YO7JANCNFSM5PRB7NOQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[ChernyshevDS]

PR is ready, you are welcome: #371

[wintal]

I've just run into this issue, is there any chance I can get this in the master branch?

[tvcong0999]

PR is ready, you are welcome: #371
I used your fix in this PR, the bug maxThreads > 1 is fixed, but CreateContext is very slow