View Issue Details

IDProjectCategoryView StatusLast Update
0003301JEDI VCL00 JVCL Componentspublic2005-11-27 03:43
ReporterKiriakosAssigned ToAHUser 
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.00 
Target VersionFixed in Version3.10 
Summary0003301: Memory loss and crashes in JvChangeNotify
DescriptionI 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.
TagsNo tags attached.

Activities

Kiriakos

2005-11-11 16:49

reporter   ~0008085

Last edited: 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.

AHUser

2005-11-22 04:28

developer   ~0008111

Fixed in CVS.
SetActive() converts every FreeOnTerminte thread into a non-FreeOnTerminate thread and destroys it immediately after a WaitFor call.

Kiriakos

2005-11-27 02:49

reporter   ~0008169

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.

Kiriakos

2005-11-27 02:52

reporter   ~0008170

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.


AHUser

2005-11-27 03:43

developer   ~0008171

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