Project JEDI - Issue Tracker
Mantis Bugtracker
Main | My View | View Issues | Change Log | Roadmap | Docs
  

Viewing Issue Simple Details [ Jump to Notes ] [ View Advanced ] [ Issue History ] [ Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0003301 [JEDI VCL] 00 JVCL Components major always 2005-11-04 06:29 2005-11-27 03:43
Reporter Kiriakos View Status public  
Assigned To AHUser
Priority normal Resolution fixed  
Status resolved   Product Version 3.00
Summary 0003301: Memory loss and crashes in JvChangeNotify
Description I discovered the following problem with FastMM4:
When the property FreeOnTerminate is True the JvChangeThread is not freed and crashes can occur.

Analysis:
Assume that in your program you so somewhere:

JvChangeNotify.Active := False;
// do something
JvChangeNotify.Active := True;

Setting Active to False will trigger the following code in TJvChangeNotify.SetActive:
    if FThread <> nil then
    begin
      FThread.Terminate;
      if FreeOnTerminate then
        FThread := nil
      else
      begin
        FThread.WaitFor;
        FreeAndNil(FThread);
      end;
    end;

Whereas Seting Active to True will create a JvChangeThread and assign it to FThread; At the same time when the original JvChangeThread gets destroyed DoThreadTerminate is called which sets FThread to nil; This may well happen after the new JvChangeThread has been created. So FThread will be nil when the JvThread is actually alive and active. Next time you set the Active property to False the JvActiveThread will not be destoyed with the result of having multiple JvChangeThreads active.

Solution: Remove DoThreadTerminate and all references to it.

Further problems:
When the JvChangeNotify gets destroyed is sets its Active property to false which will eventually destroy the JvChangeThread. However the destruction of the thread will happen after JvChangeNotify will have been destroyed. The Execute method of the thread will call its FNotify method which in turn calls the FNotify of the JvChangeNotify which will be an invalid pointer.

Suggestion:
In SetActive add the following line before calling FThread.Terminate; (2 places):
      FThread.OnChangeNotify := nil;

For futher safety, in procedure TJvChangeThread.Execute modify the 5th line to
      if (I >= 0) and (I < FCount) and not Terminated then


Even with the above suggestions there is still a chance when the Interval is long enough that TJvChangeThread.Execute will get stuck in WaitForMultipleObjects for too long and the main thread will have finished before this thread is finished.
Additional Information
Tags No tags attached.
Attached Files

- Relationships

-  Notes
(0008085)
Kiriakos (reporter)
2005-11-11 16:49
edited on: 2005-11-11 16:49

 Further to my earlier note, I found it benefitial to add a statement
Sleep(200) or even Sleep(0)
in the FormDestroy of the main application, which would give the time to the JvChangeThread (and other such threads) to terminate, so that memory loss would be avoided.
(0008111)
AHUser (developer)
2005-11-22 04:28

 Fixed in CVS.
SetActive() converts every FreeOnTerminte thread into a non-FreeOnTerminate thread and destroys it immediately after a WaitFor call.
(0008169)
Kiriakos (reporter)
2005-11-27 02:49

 Andreas,
Your solultion eliminates the difference betweeen setting the FreeOnTerminate to True or False. There was a reason for having the FreeOnTerminate flag: to be able to call SetActive from the JvChangeNotify event handler (see Peter's note on this).

Your change will break existing code resulting in the program deadlocking. Just think about it.

a) The thread execute method synchronizes into the main thread to execute the JvChangeNotify event hanldler.
b) The event handler calls SetActive(False)
c) SetActive(False) waits for the thread to terminate. Hence deadlock!

Solution:

A combination of my suggestions in earlier note and your solution would solve all problem.

a) Reverse your changes!
b) Implement the changes I suggested. (including removing DoOnTerminate which I think serves no purpose)
c) On component destruction do what you are currently doing in SetActive (set FreeOnTerminate to False, Call SetActive(False) and then WaitFor the thread to terminate if the FreeOnTerminate was True.
(0008170)
Kiriakos (reporter)
2005-11-27 02:52

 Andreas,
Your solultion eliminates the difference betweeen setting the FreeOnTerminate to True or False. There was a reason for having the FreeOnTerminate flag: to be able to call SetActive from the JvChangeNotify event handler (see Peter's note on this).

Your change will break existing code resulting in the program deadlocking. Just think about it.

a) The thread execute method synchronizes into the main thread to execute the JvChangeNotify event hanldler.
b) The event handler calls SetActive(False)
c) SetActive(False) waits for the thread to terminate. Hence deadlock!

Solution:

A combination of my suggestions in earlier note and your solution would solve all problem.

a) Reverse your changes!
b) Implement the changes I suggested. (including removing DoOnTerminate which I think serves no purpose)
c) On component destruction do what you are currently doing in SetActive (set FreeOnTerminate to False, Call SetActive(False) and then WaitFor the thread to terminate if the FreeOnTerminate was True.
(0008171)
AHUser (developer)
2005-11-27 03:43

 Previous bugfix reverted and reimplemented.

Fixed in CVS.

- Issue History
Date Modified Username Field Change
2005-11-04 06:29 Kiriakos New Issue
2005-11-11 16:49 Kiriakos Note Added: 0008085
2005-11-11 16:49 Kiriakos Note Edited: 0008085
2005-11-22 04:28 AHUser Status new => resolved
2005-11-22 04:28 AHUser Resolution open => fixed
2005-11-22 04:28 AHUser Assigned To => AHUser
2005-11-22 04:28 AHUser Note Added: 0008111
2005-11-27 02:49 Kiriakos Status resolved => feedback
2005-11-27 02:49 Kiriakos Resolution fixed => reopened
2005-11-27 02:49 Kiriakos Note Added: 0008169
2005-11-27 02:52 Kiriakos Note Added: 0008170
2005-11-27 03:43 AHUser Status feedback => resolved
2005-11-27 03:43 AHUser Resolution reopened => fixed
2005-11-27 03:43 AHUser Note Added: 0008171

Mantis 1.1.6[^]
Copyright © 2000 - 2008 Mantis Group
Powered by Mantis Bugtracker