VBA Rubberducking

Mat's Mug

22:00

2017-03-08 16:57:08.4268;ERROR-2.0.13.30090;Rubberduck.Parsing.VBA.ParseCoordinator;Unexpected exception thrown in parsing run. (thread 8).;System.Runtime.InteropServices.COMException (0x80004005): Error HRESULT E_FAIL has been returned from a call to a COM component.
at Microsoft.Vbe.Interop._VBComponent.get_CodeModule()
at Rubberduck.VBEditor.SafeComWrappers.VBA.VBComponent.get_CodeModule() in c:\Dev\Rubberduck\Rubberduck\Rubberduck.VBEEditor\SafeComWrappers\VBA\VBComponent.cs:line 25
at Rubberduck.VBEditor.QualifiedModuleName..ctor(IVBComponent component) in c:\Dev\Rubberduck\Rubberduck\…

restarting Excel

Comintern

Stale object reference in the QMN?

Mat's Mug

parsing

no idea

it was an initial parse

about to double-click the same tree node...

and everything works normally

cosmic ray

Comintern

That has to be some sort of timing issue.

Mat's Mug

I like the cosmic ray hypothesis

Comintern

That's my preferred hypothesis...

You weren't running TeamViewer were you?

Mat's Mug

22:05

nope

not even a RDP session open

Comintern

Wait a sec - RubberduckParserState.cs line 960 is inside a catch block here.

Would probably help if I synched before I started working through line numbers...

Hosch250

LOL.

M.Doerner

22:26

Looking at the functions in the call stack I start to wonder why we pass IVBComponents into these functions and not QMNs.

Comintern

It would be nice to decouple the parser from the COM objects entirely - I'm not exactly sure how to approach that though.

Mat's Mug

does a QMN count as decoupled from COM objects?

wkh,d /t <~ total keyboard off-by-one #Fail

wouldn't ICodeModule dependencies be decoupled from COM objects, technically speaking?

TTQW

Comintern

The QMN still carries a reference to an IVBComponent.

IvenBach

@Mat'sMug Try doing working with Qwerty and Dvorak keyboards. That's how you find tons of new keyboard shortcuts!

Mat's Mug

the parser can't be entirely oblivious of COM objects - it needs the VBIDE API to export the modules and reimport them back in

@IvenBach gosh... azerty is already enough to mess me up, dunno how I'd deal with dvorak

Comintern

22:36

Yeah, that's the problem. I wonder if it should be requesting them from a service provider instead of calling for them from multiple threads.

Mat's Mug

gimme-a-string

Comintern

Basically something that would have less chance of sidestepping the STA management at the Interop boundary.

Mat's Mug

in an ideal world we'd just give it a string and say "that's the content of project Id 12345, module abc"

Comintern

^

Mat's Mug

or even, "that's the content of project Id 12345, module abc, lines 48-63"

M.Doerner

22:39

I think we could really reduce the usage of IVBComponents in the RubberduckParserState. I see several calls to Component on a QMN that is just done to honor the interface of the callee that then hppilly news up a QMN from the component and uses that.

Mat's Mug

^

...wait isn't it the QMNs we want to reduce usage of?

Comintern

Yeah, I took one pass at that stuff.

@Mat'sMug Mainly because of the held COM objects.

That and the string comparisons to locate everything.

But mainly the held COM objects.

M.Doerner

I am just looking at the places where the component on the QMN is requested.

The thing is that everything in the RubberduckParserState uses QMNs as keys.

Mat's Mug

yeah... they could probably be replaced by Declaration instances though

in a lot of places we have a Declaration and fetch its QMN only to call a RPS or DeclarationFinder member

Comintern

^

M.Doerner

22:44

Declarations carry around a QMN.

Mat's Mug

yeah but we gotta start somewhere lol

if nothing uses QMN anymore, and instead relies on module declarations, we can work something out in Declaration to remove QMN

(hopefully)

M.Doerner

I do not really know whether or how often we really need the component on the QMN.

Comintern

Yeah, I discovered earlier this year that the QMN references follow a twisted path through everything. There's a bunch of overlapping functionality where just picking what should implement it would sort things out.

Mat's Mug

^

Hosch250

IIRC, QMN is only needed for the unit tests, or something.

Mat's Mug

22:46

not even

Comintern

Not anymore. Switched them to take declarations.

Mat's Mug

QMN was created because we needed a way to tell Project1.MyModule from Project2.MyModule, and the component hashcodes weren't reliable

M.Doerner

The QMNs are basically cashed information about the IVBComponents that also carry a reference to the original component.

Mat's Mug

yep

gotta run, bbl

M.Doerner

My hope would be to be able to get rid of the actual reference.

Comintern

22:48

@M.Doerner The main reason that it looks like they carry the reference is that it isn't always straight-forward to find the component again.

puzzlepiece87

Medium length class question - am I doing this right?

The user chooses custom widgets or standard widgets --> This determines if we get the widget details from the Userform or sheet.
The user chooses from 5 widget categories, and each category has two color choices. Each color choice has a different supplier.

So far my plan is to have a classWidgets with a boolean .Custom property, a string .DataSource property, a integer(long) .Category property, a string .Color property, and a string .Supplier property.

Comintern

I would love to ditch the reference. In this particular case, I suspect that something is calling the Release method of the SafeComWrapper before it should. We're basically bypassing the Interop's reference counting and forcibly releasing things when we think we're done with them.

That's a problem when we're not, or something gets final released on a thread when it should still be alive on another thread.

M.Doerner

I do not know whether that would be feasible, but we might store a dictionary QMN->component on the parser state.

Comintern

I'm curious how many of the COM errors disappear if you comment out the body of SafeComWrapper<T>.Release.

Basically, the call to Marshal.FinalReleaseComObject(Target); is the COM equivalent of while (refCount > 0) refCount--;

Hosch250

I'm curious about how many appear.

@Comintern That is a problem, yes.

If anybody else is using it (e.g. another add-in), we ruin their day.

Comintern

22:54

Especially in a multi-threaded environment.

If 2 threads are both using the same Interop object, who calls release? What happens to the thread that loses that race?

puzzlepiece87

^ Sorry folks, bad timing on that question, and good luck with Rubberduck :)

Hosch250

We should probably call Marshal.ReleaseComObject(Target) instead.

Comintern

We do. Then we call Marshal.FinalReleaseComObject(Target);

Hosch250

Why?

Comintern

    try
    {
        if (final)
        {
            Marshal.FinalReleaseComObject(Target);
        }
        else
        {
            Marshal.ReleaseComObject(Target);
        }
    }
    finally
    {
        _isReleased = true;
    }

Hosch250

22:57

Oh.

Comintern

That ended up in there before we figured out what was causing the exit crash.

Now that I have a pretty good handle on why it's really crashing on exit, I suspect that if anything that code exacerbates it.

Mat's Mug

23:27

@Comintern I put that in. by all means remove it if it's not helping anything :-)

Comintern

I don't think it makes much difference on shutdown. I tested without it in there before I left work and hit a couple weird errors, but I think they're related to other work being done in the Release overrides.

I do think the wrappers are a good idea - just not the manual reference management.

Mat's Mug

^

that was a desperate measure

Comintern

They're really convenient for tracking hard COM objects.

IvenBach

@ThunderFrame Where does this stuff come from?

Comintern

23:44

Huh. Excel doesn't attempt to restart after the exit crash after commenting out all the Release related code.

IvenBach

@Mat'sMug Will you be available for my continued instruction on Lexer, Parser, and what ever else I need to learn?

Mat's Mug

@IvenBach after the kids are in bed, in about 1.5hrs

@Comintern and that's... Good I guess?

IvenBach

Ok. I really do appreciate you taking this time to instruct me.

Comintern

@Mat'sMug TBH, I have no clue what the trigger for an Excel restart is. It is one less dialog to dismiss though...

Transcript for