A coworker and I ran into this code in some of our WinForms in our Compact Framework (.net 3.5) application:
1: public class LaunchForm: Form
3: //a bunch of other form/view related stuff
5: private void Login()
7: LoginForm loginForm = new LoginForm();
8: loginForm.Owner = this;
9: DialogResult result = loginForm.ShowDialog();
10: DialogResult = result;
14: public class LoginForm: Form
16: //a bunch of other form/view related stuff
18: private void Completed()
20: DialogResult = DialogResult.OK;
21: ShowRolesForm form = new ShowRolesForm();
Can you spot the bug? No, it’s not some style issue or architectural best practice… there is an application-crashing bug in this code.
A Memory Leak And An Exception
According to the MSDN documentation, setting the DialogResult of a dialog modal window will cause the ShowDialog call to return the value but will not cause the form to close. Instead, it just hides the form. Since the LoginForm is immediately calling out to the ShowRolesForm form after setting the DialogResult, the LoginForm is still held in memory because the ShowRolesForm form is blocking the LoginForm’s thread and it can’t exit.
This causes two problems:
- Memory Leak: Until ShowRolesForm closes or returns a DialogResult, LoginForm is held in memory… if ShowRolesForm is the main window that the user interacts with, you essentially have a memory leak with the LoginForm still being around
- Exception: ShowDialogForm still returns it’s result immediately when LoginForm set it’s own DialogResult value. When LaunchForm receives that value it tries to set it’s own DialogResult value. But since LoginForm is still alive and is still parented by LaunchForm, LaunchForm is not allowed to set a dialog result yet – the runtime thinks that LoginForm is still the top most dialog stack which causes LaunchForm to throw an exception when it tries to set it’s DialogResult, crashing the app.
Decouple Your Workflow From Your Forms
The solution to this situation is fairly straightforward: decouple the workflow of your system from the forms in your system. I’ve talked about this process a number of times on my blog and you can read all my posts on the Application Controller for information on how to go about doing this from a larger perspective.
In this particular case, a simple object to control the flow between LaunchForm, LoginForm and ShowRolesForm will do what we need. Each form should make sure to .Close() itself before setting the DialogResult.
1: public class SomeWorkflowService
3: public void Run()
5: var launchForm = new LaunchForm();
6: var launchResult = launchForm.ShowDialog();
9: //do something to check the launchResult here and determine
10: //if we need to go on
12: var loginForm = new LoginForm();
13: var loginResult = loginForm.ShowDialog();
16: //do something to check the loginResult here and determine
17: //if we need to go on
19: var showRolesForm = new ShowRolesForm();
20: var showRolesResult = showRolesForm.ShowDialog();
23: //do something with the showRolesResult here
If you are using a form management class (something I haven’t talked about yet, but am planning to do), make sure the form manager is calling .Dispose() on the form that returned a dialog result.