Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need more cases of ObjectDisposedException #3675

Open
tig opened this issue Aug 20, 2024 · 5 comments · May be fixed by #3676
Open

Need more cases of ObjectDisposedException #3675

tig opened this issue Aug 20, 2024 · 5 comments · May be fixed by #3676
Assignees
Labels
Milestone

Comments

@tig
Copy link
Collaborator

tig commented Aug 20, 2024

I just spent several hours fighting bugs in Menubar and ContextMenu only to discover that they are passing around views (and themsevles) that have been disposed.

We have a lot of this going on and we need to squash it.

This issue is to track adding a bunch of calls like this in key places:

#if DEBUG_IDISPOSABLE
            if (WasDisposed)
            {
                throw new ObjectDisposedException (GetType ().FullName);
            }
#endif
  • Visible get/set
  • Enabled get/st
  • HasFocus get/set
  • Mouse event handling
@tig tig added the bug label Aug 20, 2024
@tig tig added this to the V2 Beta milestone Aug 20, 2024
@tig tig self-assigned this Aug 20, 2024
@tig tig linked a pull request Aug 21, 2024 that will close this issue
13 tasks
@dodexahedron
Copy link
Collaborator

Just a reminder:

Don't forget to use the handy-dandy ObjectDisposedException.ThrowIf helper instead of manually constructing and throwing, too!

Better static analysis and optimization, with a little less code, so a win all around!

The code in the initial post would become:

ObjectDisposedException.ThrowIf(WasDisposed,typeof(whateverClassThisIsIn));

Don't use the overload that takes an object instance, as it uses reflection to get the type name (which is all the second argument is for). It calls GetType() at run-time on whatever you passed. typeof(SomeType) is a compile-time constant, which avoids that and is more efficient, to boot. 🥳

@dodexahedron
Copy link
Collaborator

dodexahedron commented Aug 21, 2024

Also, it's typically not thrown from property accessor methods, unless the accessor itself accesses a different disposable object that has been disposed, and it is supposed to leave the throwing of that up to that object. Basically, it's considered API consumer error, and they're pretty likely to get a NullReferenceException anyway. Plus, guidelines are to avoid throwing any exceptions from property accessors, directly, and that, if you feel the need to, the property should be turned into explicit methods instead.

Methods, however, are expected to throw, ideally with a more detailed InnerException if possible/relevant.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Aug 21, 2024

A couple of heads ups before you write a whole bunch of code, just in case I didn't beat this particular horse enough or properly last time we did exception work (plus I'm pretty sure I learned some nuance already included in the above since then, as well). 😄

@dodexahedron
Copy link
Collaborator

Oooo

And for methods that return IDisposables, and any types that are themselves IDisposable, it's friendly to include the [MustDisposeResource(bool)] attribute, with true if the caller is expected to dispose it or false if it doesn't matter who disposes it.

@tig
Copy link
Collaborator Author

tig commented Aug 21, 2024

Here's the issue I found that led to this exercise:

#3678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 📋 Approved - Need Owner
Development

Successfully merging a pull request may close this issue.

2 participants