Ribbon Sample: Excel crashes after closing

[hell-racer]

1. Build the Ribbon Sample addin (Debug or Release)
2. Open Ribbon-AddIn64-packed.xll with Excel 2016 x64
3. Click the "My Button" button
4. Close Excel:

Tried on Excel 2016 x64 Version 1803 (Build 9126.2152 Click-to-Run).
Tried to update ExcelDna NuGet packages to v0.34.6.

If I don't click My Button and just close Excel right away - it doesn't crash.

[hell-racer]

Fixed via #9

[govert]

I could not recreate this problem (including on 64-bit Excel).

[hell-racer]

Do you wait after you close Excel? Does the Excel process unload?

[govert]

Yes. It takes a few seconds and then it closes.

[hell-racer]

Do you build it with VS 2015? Maybe build environment is different in some way... Can you try with the attached built xll?

I just can't comprehend... It reproduces on my dev PC and two VMs each and every time :). All of them have Windows 10 and Office 2016 x64 (two Office 365 and one ProPlus).

Ribbon-AddIn64-packed.zip

[govert]

Never crashes.
First time Excel process didn't seem to end, all the subsequent times Excel ends after a few seconds.

[govert]

Other add-ins running?

[hell-racer]

I had some of them enabled, but tried to disable all of them and still have a crash.

[hell-racer]

Ok, I finally got one PC when the crash doesn't seem to appear.
But, I still get an error in the Windows Logs -> Application:

Faulting application name: EXCEL.EXE, version: 16.0.9226.2156, time stamp: 0x5af64083
Faulting module name: ucrtbase.dll, version: 10.0.17134.1, time stamp: 0x587decd7
Exception code: 0xc0000409
Fault offset: 0x000000000006e75e
Faulting process id: 0x5270
Faulting application start time: 0x01d3f7f00a6b1f0e
Faulting application path: C:\Program Files\Microsoft Office\Root\Office16\EXCEL.EXE
Faulting module path: C:\WINDOWS\System32\ucrtbase.dll
Report Id: 887877a1-f4b6-469d-9fe4-4d7be771e806
Faulting package full name:
Faulting package-relative application ID:

The same error appears in the logs on other machines where I actually get the message about the crash.
Could you please check your logs?

[govert]

Yes - I see the same in the event log

Not sure what to think about it.
One place that there are sometimes bugs in the COM object model implementation is when it expects to have the objects released in a certain order, but not keeping internal references to ensure that it is. Since whole charting implementation was redone recently (2007 / 2010?) there might be bugs like that lurking. But that's pure speculation.

I'd suggest trying to recreate the crash in VBA.

[govert]

Actually looks like a null reference. This is with a debugger attached:

Exception thrown at 0x00007FF82229C423 (Mso20win32client.dll) in EXCEL.EXE: 0xC0000005: Access violation writing location 0x0000000000000000.
The Common Language Runtime cannot stop at this exception. Common causes include: incorrect COM interop marshalling and memory corruption. To investigate further use native-only debugging.
Unhandled exception at 0x00007FF8463FB79E (ucrtbase.dll) in EXCEL.EXE: Fatal program exit requested.
The Common Language Runtime cannot stop at this exception. Common causes include: incorrect COM interop marshalling and memory corruption. To investigate further use native-only debugging.
Exception thrown at 0x00007FF666671350 in EXCEL.EXE: 0xC0000005: Access violation writing location 0x0000000000000000.

[hell-racer]

It seems like this is definitely somehow related to COM objects releasing... If I modify the OnButtonPressed method as you suggest:

public void OnButtonPressed(IRibbonControl control)
{
	MessageBox.Show("Hello from control " + control.Id);
	DataWriter.WriteData();

	do
	{
		GC.Collect();
		GC.WaitForPendingFinalizers();
	}
	while (Marshal.AreComObjectsAvailableForCleanup());
}

... it doesn't crash :).

[govert]

Ugh. I suppose you could try that in the ribbon OnDisconnect callback.

I tried to fiddle in VBA a bit, but can't run into problems there. Maybe with a VSTO add-in...
(I suspect one would need something without Excel-DNA in the loop to report to Microsoft. I keep looking for some bug I can report to then to see how the process works, but mostly I run out of patience too soon.)

[hell-racer]

Yeah, that code placed into OnDisconnection works as well (I mean Excel doesn't crash after close).
Should we modify the Ribbon Sample project with this?

Yeah, last time my colleague tried to report something to MS it took about a year to narrow down the problem and another year to realize that the fix will break other things and therefore it's better to leave it as is. And yes, that was related to COM too :).

[rkapl123]

As I had a similar problem with Excel crashing after being closed, I wanted to share some insights I gathered around this and my solution.
For me it was only related to COM Objects not being released properly at least on shutdown.
So I have followed Goverts suggestion to use the OnDisconnection method (here Functions.StatusCollection and DBModifDefColl are collections of dynamically allocated objects containing references to excel ranges, which are then explicitly released using Marshal.ReleaseComObject):

Public Class MenuHandler
    Inherits CustomUI.ExcelRibbon

    Public Overrides Sub OnDisconnection(RemoveMode As ExcelDna.Integration.Extensibility.ext_DisconnectMode, ByRef custom As Array)
        For Each aKey As String In Functions.StatusCollection.Keys
            Dim clearRange As Excel.Range = Functions.StatusCollection(aKey).formulaRange
            If Not IsNothing(clearRange) Then Marshal.ReleaseComObject(clearRange)
        Next
        For Each DBmodifType As String In DBModifDefColl.Keys
            For Each dbmapdefkey As String In DBModifDefColl(DBmodifType).Keys
                Dim clearRange As Excel.Range = DBModifDefColl(DBmodifType).Item(dbmapdefkey).getTargetRange()
                If Not IsNothing(clearRange) Then Marshal.ReleaseComObject(clearRange)
            Next
        Next
        Do
            GC.Collect()
            GC.WaitForPendingFinalizers()
        Loop While (Marshal.AreComObjectsAvailableForCleanup())
    End Sub
...

I have left the GC.Collect in there just for safety if the GC didn't clean up all COM objects properly.
After applying this, my Addin didn't crash Excel no more.

Thank you Govert.

[rkapl123]

Another reason that I have uncovered now: Any shared or static class variables should be released on finalizing the class, I have some of these in the AddInEvents class used for handling events:

<ComVisible(True)>
Public Class AddInEvents
    Implements IExcelAddIn

    'CommandButton that can be inserted on a worksheet 
    Shared WithEvents cb1 As Forms.CommandButton
    'CommandButton that can be inserted on a worksheet 
    Shared WithEvents cb2 As Forms.CommandButton
    'CommandButton that can be inserted on a worksheet 
    Shared WithEvents cb3 As Forms.CommandButton
...

    Protected Overrides Sub Finalize()
        MyBase.Finalize()
        If Not IsNothing(cb1) Then Marshal.ReleaseComObject(cb1)
        If Not IsNothing(cb2) Then Marshal.ReleaseComObject(cb2)
        If Not IsNothing(cb3) Then Marshal.ReleaseComObject(cb3)
...
    End Sub
End Class

I know this is a rather advanced topic, but sooner or later someone will run into those, so maybe this should be documented somewhere...