Form Management: Tight Or Incorrect Coupling Can Cause Hard To Find Bugs

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

   2: {

   3:     //a bunch of other form/view related stuff

   4:  

   5:     private void Login()

   6:     {

   7:         LoginForm loginForm = new LoginForm();

   8:         loginForm.Owner = this;

   9:         DialogResult result = loginForm.ShowDialog();

  10:         DialogResult = result;

  11:     }

  12: }

  13:  

  14: public class LoginForm: Form

  15: {

  16:     //a bunch of other form/view related stuff

  17:     

  18:     private void Completed()

  19:     {

  20:         DialogResult = DialogResult.OK;

  21:         ShowRolesForm form = new ShowRolesForm();

  22:         form.ShowDialog();

  23:     }

  24: }

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:

  1. 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
  2. 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

   2: {

   3:     public void Run()

   4:     {

   5:         var launchForm = new LaunchForm();

   6:         var launchResult = launchForm.ShowDialog();

   7:         launchForm.Dispose();

   8:         

   9:         //do something to check the launchResult here and determine

  10:         //if we need to go on

  11:         

  12:         var loginForm = new LoginForm();

  13:         var loginResult = loginForm.ShowDialog();

  14:         loginForm.Dispose();

  15:         

  16:         //do something to check the loginResult here and determine

  17:         //if we need to go on

  18:         

  19:         var showRolesForm = new ShowRolesForm();

  20:         var showRolesResult = showRolesForm.ShowDialog();

  21:         showRolesForm.Dispose();

  22:         

  23:         //do something with the showRolesResult here

  24:     }

  25: }

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.


Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Derick Bailey

Derick Bailey is an entrepreneur, problem solver (and creator? :P ), software developer, screecaster, writer, blogger, speaker and technology leader in central Texas (north of Austin). He runs SignalLeaf.com - the amazingly awesome podcast audio hosting service that everyone should be using, and WatchMeCode.net where he throws down the JavaScript gauntlets to get you up to speed. He has been a professional software developer since the late 90's, and has been writing code since the late 80's. Find me on twitter: @derickbailey, @mutedsolutions, @backbonejsclass Find me on the web: SignalLeaf, WatchMeCode, Kendo UI blog, MarionetteJS, My Github profile, On Google+.
This entry was posted in .NET, AntiPatterns, AppController, C#, Compact Framework, Principles and Patterns. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • David

    I would suggest the use of the using statement instead of directly calling Dispose. That way, if any of the forms do cause an exception, the form will still be disposed of cleanly.