Crash prevention

[user5]

In my Lazarus program, but only every once in a long while, the program will crash totally and self-terminate without even an exception message and I tracked the error down to the following statement:

"Image6.picture.LoadFromFile(directory+'temp1.bmp');"

It only happens when Image6 already contains a previous .bmp. It NEVER crashes when Image6 is loaded for the first time. I don't know if it's necessary with TPicture, but I do include create and free statements so I don't think that's the reason for the crash.

Do you guys think the following code, updated due to the crashing, will help, or do you know a better way to load a bitmap into a TImage? It compiles and works, though it may look redundant. I thought that I'd try to combine several error checking methods.

newerror := false;
Image6.picture.clear;//Clear TImage?
if FileExists(directory+'temp1.bmp') then
  begin
       try
         try
          begin
           {$I-}
           testtbitmap.LoadFromFile(directory+'temp1.bmp');
           {$I+}
            if IOResult <> 0 then
             newerror := true;
            if newerror = false then
             Image6.picture.Graphic:=testbitmap;
           end
         except
          On E: Exception do
           begin
            sound(350);
            newerror := true;
           end;
       end;
      finally
       ;//Do nothing here.
     end;
  end
 else
  newerror := true;

   if newerror then
    begin
     newerror := false;//Change to default.
     form95.show;//Show error message.
     application.processmessages;
    end;

[Thaddy]

Why are you mixing structured exception handling with IOResult style error handling?
That is normally  not very good practice.
btw: why try/finally and do nothing: doesn't make sense, try finally is no guard, it only ensures the finally part gets executed.

[taazz]

I doubt it help. You tracking might not be complete. Start by describing how you came to the conclusion that the code you are showing is the problematic code and not something else and then remove from your code all empty try finally blocks. An empty try Finally is the same as

Code: [Select]

  try
  except
    raise;
  end;
it protects you from nothing its a waste of screen estate.

[Thaddy]

posts crossed, but they are not the same. They are in effect the same under most circumstances.

[taazz]

posts crossed, but they are not the same. They are in effect the same under most circumstances.

I'm trying to show the "logic" not the details. The details are different yes and on top of that what ever is inside the finally..end block will be executed even if there is no exception raised but if it is an empty try finally then it has no real reason to exists.

EDIT: I have seen this kind of code from vb6 developers that try to emulate the "on error continue" command and the above piece of code usually is a quick way to showcase my point.

[user5]

Thanks for your comments. They were all very interesting and I will certainly consider what you all said. This particular error is maddening because it only happens every once in a long while.

taazz, your perceptive comment about whether the crash happened because of the statement shown or because of some prior event, also occurred to me, so in the procedure where the error happens I sprinkled variations of the following two statements through the code:

testline := 20198;
TestForCrash;

testline is a variable that is printed to a text file by means of TestForCrash and the changing number represents a line of code. If the procedure crashes, all I have to do is to look at that file to see the last number printed, knowing that whatever follows that number in the code is what caused the crash.

My only other clue is the fact that this crash never happens on the first run through the procedure. I'm sorry to admit that I don't know how to use the Lazarus compiler to do this sort of error tracking. I'm a pretty good programmer and I'm pretty excited about some code stuff that I'm doing but I know there are people who can run circles around me.

I also admit that the "finally;//do nothing" line appears superfluous but, like you said, it at least "guarantees" that the program will continue to run instead of crashing or posting an exception message. Exception messages usually offer a choice of canceling and ending the program or of continuing while risking data corruption but in this case I've noticed that every time I selected to continue, the program ran fine after that so I don't want a user of my program to even be faced with a situation where he or she might make the wrong and unnecessary choice (at least in this case).

The hard-to-find cause of this error should be solved but if I can't solve it, then I want the program to at least continue running, especially considering the fact that this is a very rare error. I've never yet failed to solve an error (knock on wood) and I will probably find the cause of this bad bug eventually, I hope.

By the way, when I hit 100 posts, will I move up from being a Jr. Member? Not that I'm not proud of that, of course.
Thanks again!

[taazz]

Thanks for your comments. They were all very interesting and I will certainly consider what you all said. This particular error is maddening because it only happens every once in a long while.

taazz, your perceptive comment about whether the crash happened because of the statement shown or because of some prior event, also occurred to me, so in the procedure where the error happens I sprinkled variations of the following two statements through the code:

testline := 20198;
TestForCrash;

testline is a variable that is printed to a text file by means of TestForCrash and the changing number represents a line of code. If the procedure crashes, all I have to do is to look at that file to see the last number printed, knowing that whatever follows that number in the code is what caused the crash.

My only other clue is the fact that this crash never happens on the first run through the procedure. I'm sorry to admit that I don't know how to use the Lazarus compiler to do this sort of error tracking. I'm a pretty good programmer and I'm pretty excited about some code stuff that I'm doing but I know there are people who can run circles around me.

I also admit that the "finally;//do nothing" line appears superfluous but, like you said, it at least "guarantees" that the program will continue to run instead of crashing or posting an exception message.

You misunderstood me I said the opposite. the only thing that guarantees is that what ever code is between finally and end is executed and then the program crashes or an exception message is shown. eg

Code: [Select]

try
  raise exception.create('this is a dummy exception');
finally                                            //<-- guarantee starts
  Writeln('Clean up as expected');
end;                                               //<-- guarantee ends exception is raised.
  Writeln('this is will never happen'); //never going to execute

in short an empty "try finally end" block guarantees that what ever problem you are having, it will crash your application.

The hard-to-find cause of this error should be solved but if I can't solve it, then I want the program to at least continue running, especially considering the fact that this is a very rare error. I've never yet failed to solve an error (knock on wood) and I will probably find the cause of this bad bug eventually, I hope.

By the way, when I hit 100 posts, will I move up from being a Jr. Member? Not that I'm not proud of that, of course.
Thanks again!

I do not remember sorry.

[rvk]

Your optimized code would look something like this:

Code: [Select]

  Image6.Picture.Clear; // Clear Image
  if FileExists(Directory + 'temp1.bmp') then
    try
      Image6.Picture.LoadFromFile(Directory + 'temp1.bmp');
    except
      On E: Exception do
        Showmessage('Error = ' + E.Message);
    end;

But I do need to ask... the code you gave in your first post... does that come straight from your program? I don't think so !!! You have testtbitmap as first variable and later on testbitmap. Notice the extra t? So you gave us code which doesn't actually work. Not very useful.

Also... at what point is this code executed? Is it in an event?

Because you say your program even ends without an exception there must be something else going on. Without more of your code it's impossible for us to say what. The fact your code runs exactly until the LoadFromFile does not necessarily mean the problem is actually in the LoadFromFile.

[Basile B.]

Code: [Select]

Image6.picture.Graphic:=testbitmap;
it looks like the the assignment operator is used in the wrong way.
With classes, := replaces the target instance (and the previous instance leaks, and some references to this previous instances are not invalidated). Use assign() instead, to copy the data from one instance to another:

Code: [Select]

Image6.picture.Graphic.assign(testbitmap);

[taazz]

Code: [Select]

Image6.picture.Graphic:=testbitmap;
it looks like the the assignment operator is used in the wrong way.
With classes, := replaces the target instance (and the previous instance leaks, and some references to this previous instances are not invalidated). Use assign() instead, to copy the data from one instance to another:

Code: [Select]

Image6.picture.Graphic.assign(testbitmap);

no not really this is a property not a variable it heavely depends on the setter method, in this case you are 100% wrong not only it does not "leak" the old instance it does a wonderful job to copy the class as well instead of converting the new class to the old class that your code will probably do.

[Basile B.]

It does that

Code: [Select]

procedure TPicture.SetGraphic(Value: TGraphic);
var
  NewGraphic: TGraphic;
  ok: boolean;
begin
  if (Value=FGraphic) then exit;
  NewGraphic := nil;
  ok := False;
  try
    if Value <> nil then
    begin
      NewGraphic := TGraphicClass(Value.ClassType).Create;
      NewGraphic.Assign(Value);
      NewGraphic.OnChange := @Changed;
      NewGraphic.OnProgress := @Progress;
    end;
    FGraphic.Free;
    FGraphic := NewGraphic; //!\ new heap address, if TPicture.Graphix is stored elsewhere than this storage points to an invalid instance /!\
    Changed(Self);
    ok := True;
  finally
    // this try..finally construction will in case of an exception
    // not alter the error backtrace output
    if not ok then
      NewGraphic.Free;
  end;
end;

If TPicture.Graphic is stored elsewhere then the "elsewhere" becomes invalid when it's reassigned using the setter (its address changes). Using assign(), .Graphic address does not vary.

However i'd like to see a reduced code that reproduces the problem. With his sample it's hard to say if it's relevant or not.

However you're right, there's absolutely no leak. I should check before...

[taazz]

If TPicture.Graphic is stored elsewhere then the "elsewhere" becomes invalid when it's reassigned using the setter (its address changes). Using assign(), .Graphic address does not vary.

However i'd like to see a reduced code that reproduces the problem. With his sample it's hard to say if it's relevant or not.

ok lets break things down a bit

Pass the component reference by value not by reference even if value changes the original graphic is not lost.

Code: [Select]

procedure TPicture.SetGraphic(Value: TGraphic);
var
  NewGraphic: TGraphic;
  ok: boolean;
begin
  if (Value=FGraphic) then exit;

if the value passed is FGraphic then don't waste time and cpu.

Code: [Select]

  NewGraphic := nil;
  ok := False;
  NewGraphic is a local variable and the above section initializes the values.

Code: [Select]

  try
    if Value <> nil then
    begin
      NewGraphic := TGraphicClass(Value.ClassType).Create;
  If the value is not nil make sure that a new graphic of the same class (also known as image type) is created

Code: [Select]

      NewGraphic.Assign(Value);
  since the same image type has been created the assign procedure does not convert the passed graphic (eg it does not convert a png to a bmp)

Code: [Select]

      NewGraphic.OnChange := @Changed;
      NewGraphic.OnProgress := @Progress;
    end;
  set some internal management values nothing to do with the assignment process.

Code: [Select]

    FGraphic.Free;
  free the old graphic. so far the value parameter has not changed at all and no leaks happened.

Code: [Select]

    FGraphic := NewGraphic; //!\ new heap address /!\
  fgraphic at this point has an invalid reference so make sure it has a valid one.

Code: [Select]

    Changed(Self);
    ok := True;
  raise the appropriate events and invalidate the needed objects.

Code: [Select]

  finally
    // this try..finally construction will in case of an exception
    // not alter the error backtrace output
    if not ok then
      NewGraphic.Free;
in case of an exception cleanup things of course if the exception happens after the fgraphic.free line then we left the component in an invalid unrecoverable state.

Code: [Select]

  end;
end;

That is what I see here there is no leak or memory corruption and certainly there is no orphaned reference. An invalid reference in the FGraphic might come up depending on the exception and the position it happened and can be solved easily by using the freeandnil function instead of calling free directly or by using the interlockedexchange to set the value to nil before freeing the fgraphic.

Since the program has a lot of  try finally and I assume the same amount or more of empty "try except" blocks the actual exception is lost and we do not know what to look for.
I am with you that this seems a lot like a buffer overrun or some other kind of memory corruption which happens long before the process reaches the specified procedure but the error is silenced instead of showing it to the end user.

PS:
I sometimes become extremely thick headed, if thats the case here too I expect some smacking around to get me started again.

[Basile B.]

There is no problem. When i realized i've spoken too far i've looked at the code. So no leak, we agree. However FGraphic might be "escaped" and this can be the source of an error in the user code because of

Code: [Select]

Image6.picture.Graphic:=testbitmap;

Assuming that he has escaped Graphic, which can become invalid since it returns FGraphic (which is mutable), example:

Code: [Select]

MyRef := Image6.picture.Graphic;
Image6.picture.Graphic:=testbitmap;
// now MyRef might point to something that no longer exists !


[user5]

Wow! Thanks so much for your responses, though some of what you say is over my head.

Thanks for your input taazz. Do you think the code below will help?

rvk, I'm going to use your suggested code plus BBasile's suggestion to use "Assign". I know that something prior to the 'problem' statement may be causing the error and I hate unresolved stuff like this. I may post the whole procedure if you want. I would need to 'clean up' the procedure and put comments into into it to explain what the code is doing before I could post it, not that you guys wouldn't be able to understand it. Yes, the code I posted is in my program and it runs but I'm going to replace it with the snippet below, based on your suggestions. The extra "t" in testbitmap from my previous post is a typo transcribing error. Sorry about that. It happens to everyone now and then.

I use AProcess a lot and sometimes I wonder if that may have something to do with the error. It seems as though those AProcesses don't always finish before the next code is executed, even with a "poWaitOnExit" command and that could mean that any graphics or other stuff created by AProcess may not be fully developed and ready when they need to be loaded, which might cause problems. I've even had to use delays in regard to this issue and I don't like using delays but right now I'm only guessing.

The procedure in which this error happens loads a sound file, then makes a graphic waveform image of that sound   file and then loads that picture. I can't reproduce the error but on those very rare occasions when it happens, it always happens after I first load an audio file and then right away load a different audio file.

  testbitmap := TBitmap.Create;
  Image6.Picture.Clear;//This might help.
  if FileExists(Directory+'temp1.bmp') then
    try
     begin
      testbitmap.LoadFromFile(Directory+'temp1.bmp');
      Image6.Picture.Graphic.assign(testbitmap);
     end
    except
      On E: Exception do
        Showmessage('Error = ' + E.Message);
    end;

  testbitmap.Free;

[rvk]

rvk, I'm going to use your suggested code plus BBasile's suggestion to use "Assign".

Why would you want to use the extra testbitmap variable? You could just use Image6.Picture.LoadFromFile directly and in that case you wouldn't have to use assign().

The extra "t" in testbitmap from my previous post is a typo transcribing error. Sorry about that. It happens to everyone now and then.

I usually type small snippet-code in Lazarus itself and try if it runs. In that case no typos are made (with a rare exception).
B.T.W. You might want to use the [ code ] [ /code ] tags around your code. It a lot easier to read.

The procedure in which this error happens loads a sound file, then makes a graphic waveform image of that sound   file and then loads that picture. I can't reproduce the error but on those very rare occasions when it happens, it always happens after I first load an audio file and then right away load a different audio file.

Ok, so if I understand correctly you have a external application which you call with AProcess and which generates your temp1.bmp. After that you directly want to load that temp1.bmp into your image6. That shouldn't be a problem (normally). Let see if your live application produces a real exception at that point.

There is no need for the begin and end in the try/except block. try/except and try/finally are already block indicators so there is no need to do begin/end inside them.

Code: [Select]

  testbitmap := TBitmap.Create;
  Image6.Picture.Clear;//This might help.
  if FileExists(Directory+'temp1.bmp') then
    try
     begin               // <---- NO NEED FOR BEGIN BECAUSE OF THE TRY/EXCEPT BLOCK
      testbitmap.LoadFromFile(Directory+'temp1.bmp');
      Image6.Picture.Graphic.assign(testbitmap);
     end               // <---- NO NEED FOR END BECAUSE OF THE TRY/EXCEPT BLOCK
    except
      On E: Exception do
        Showmessage('Error = ' + E.Message);
    end;

  testbitmap.Free;


If you really want to use your assign snippet (with the testbitmap) I would do it like this:

Code: [Select]

  Image6.Picture.Clear;//This might help.
  if FileExists(Directory + 'temp1.bmp') then
    try
      testbitmap := TBitmap.Create;
      try
        testbitmap.LoadFromFile(Directory + 'temp1.bmp');
        Image6.Picture.Graphic.Assign(testbitmap);
      finally
        testbitmap.Free;
      end;
    except
      On E: Exception do
        ShowMessage('Error = ' + E.Message);
    end;

In that case you only create the temporary testbitmap when the temp1.bmp exists. The finally is always executed so you don't get a memory leak when an exception occurs.

(But like I said... if there is no real need for the testbitmap I would do a Image6.Picture.LoadFromFile directly)