View Issue Details

IDProjectCategoryView StatusLast Update
0025750LazarusDatabasepublic2014-03-03 20:18
ReporterStratis AraviasAssigned ToJesus Reyes 
PrioritynormalSeverityminorReproducibilityhave not tried
Status resolvedResolutionfixed 
PlatformOSWindowsOS Version7
Product VersionProduct Build 
Target Version1.4Fixed in Version1.3 (SVN) 
Summary0025750: memory leak in TDBImage
Descriptionmemory leak detected in TDBImage.LoadPicture procedure when your underlying dataset has no images.
Steps To Reproducei use SQLite as db and a table with a blob field for storing images. So make a form with TDBNavigator and the TDBImage component and the rest necessary components for connecting to underyling table (connection,transaction,query,datasource)

Then just create a few records (or one) and just navigate on them without inserting any image. When i shut down the application i get report for memory leaks that refer also this procedure.
Additional InformationTaking a closer look in TDBImage.LoadPicture (dbimage.inc) i see that in line 151 we have an exit procedure command, without Free the s variable.
I suggest to add the following line just before "exit;" command on line 151:

150: Picture.Clear;
ADD THIS 151: if(s<>nil)then s.Free;
152: exit;

which seems to fix my issue;
Tagsmemory leak, TDBImage
Fixed in Revision44334
LazTarget1.4
Widgetset
Attached Files
  • dbimage_memleak.diff (331 bytes)
    Index: dbimage.inc
    ===================================================================
    --- dbimage.inc	(revision 44196)
    +++ dbimage.inc	(working copy)
    @@ -183,6 +183,7 @@
             if (S=Nil) or (s.Size = 0) then
               begin
               Picture.Clear;
    +          if (S<>Nil) then s.Free;
               exit;
               end;
     
    
    dbimage_memleak.diff (331 bytes)

Activities

Reinier Olislagers

2014-02-21 13:12

developer   ~0073190

This is a Lazarus issue; devs: please reassign to Lazarus/DB controls.

@Stratis Aravias: no idea what Lazarus version you're using (that's why it's always a good idea to send in a patch, if possible)... I hope your code snippet corresponds to Lazarus trunk r44196, line 182 and further, your proposal makes sense:

        s := FDataLink.DataSet.CreateBlobStream(FDataLink.Field,bmRead);
        if (S=Nil) or (s.Size = 0) then
          begin
          Picture.Clear;
          if (S<>Nil) then s.Free;
          exit;
          end;

See attached patch

Reinier Olislagers

2014-02-21 13:13

developer  

dbimage_memleak.diff (331 bytes)
Index: dbimage.inc
===================================================================
--- dbimage.inc	(revision 44196)
+++ dbimage.inc	(working copy)
@@ -183,6 +183,7 @@
         if (S=Nil) or (s.Size = 0) then
           begin
           Picture.Clear;
+          if (S<>Nil) then s.Free;
           exit;
           end;
 
dbimage_memleak.diff (331 bytes)

Stratis Aravias

2014-02-21 14:21

reporter   ~0073192

My "About form" in Lazarus says...

Version #:1.0.14
Date 2014-02-19
FPC Version:2.6.2
SVN Revision 43446
i386-win32-win32/win64

so i have older version...

anyway as i said the problem is with the s variable which is allocated in the
.CreateBlobStream, but not released in case of an empty Blob stream, due to
exit comand (release happens later in a try-finally which seems to be declared
for other object).

I proposed this as a quick fix, which seems to solve my leak issue and seems that it doesn't create new problems.

You can also rewrite the code for the proc with a 2nd try-finally (i think we must always use a seperate try-finally block for each seperated allocated resource, but here with the exit command... i don't know the behaviour, and have no time for tests)

My patch should fail only if Picture.Clear throws an exception for some other reason, so theoritically speaking the patch doesn't eleminate all possibillities of leaking and it may need to review again proc's code ...

How can i reassign the issue?

Reinier Olislagers

2014-02-21 15:00

developer   ~0073195

Ok, thanks - then it seems we're on the same page/are talking about the same code.

FYI: you can see the current trunk code via
http://svn.freepascal.org/svn/lazarus/trunk/lcl/include/dbimage.inc

You cannot reassign the issue; it requires somebody with the appropriate permissions.

Dennis Fehr

2014-02-21 18:33

reporter   ~0073198

Last edited: 2014-02-21 18:35

View 2 revisions

Calling 'exit' will call all 'finally' blocks from where it was surrounded:

----------

A := TA.Create;

Try
  B := TB.Create;
  Try
    Exit;
  Finally
    B.Free;
  End; { Try, Finally }
Finally
  A.Free;
End; { Try, Finally }

--------------------

In this case, B.Free and A.Free will be called. This is due to the *cough* massive amount of Jump Labels in the Assembly.. *cough* .. so many.. labels.. There really should be a unused labels post-process for assembly output. :p

Marco van de Voort

2014-02-21 21:39

manager   ~0073201

Reinier: what is the difference between x.free and x.destroy ?

(hint: why is if (S<>Nil) then s.Free; not sane?)

Stratis Aravias

2014-02-22 00:32

reporter   ~0073202

Last edited: 2014-02-22 00:43

View 2 revisions

@Dennis Fehr: nice i know they will be called... but when? after exit or before exit? because if they called after, are B,A valid vars ?

@Marco: sorry, i proposed the fix (for my version), that doesn't make things worst (from what they already are), just eleminates the leak in case of allocating a stream for the (s) local variable.

As i can see in TObject.Free online documentation. freepascal doesn't check for nil (http://www.freepascal.org/docs-html/rtl/objects/tobject.free.html), while
lazarus online documentation says that TObject.Free checks for nil (http://lazarus-ccr.sourceforge.net/docs/rtl/system/tobject.free.html).

Anyway who guaranties me that the Free method is not overriden in descedants and it's behaviour is different there, and it will be the same for ever? ... no one.
So based on this i check the s variable for nil, and then call its method (i didn't knew that you can call a method on a nil).

"Not sane.."? should we make it nil to avoid any clean up from proc exit?

The problem here (as i see it) in the proc is that it allocates a resource on s var and calls exit without freeing the resource (compiler doesn't clean up s var on exit).
If you see later (on line 230) it tries to free the resource in a try-finally that is written for another resource (actually the AGraphic)... which in my poor opinion is the "not ...sane".

Reinier Olislagers

2014-02-25 11:13

developer   ~0073281

Last edited: 2014-02-25 11:25

View 2 revisions

@Marco: I don't think this is the right venue for educational discussions. Please fix however you see fit.
If you don't have Laz commit rights, please provide a patch with the code you think is right.

Thanks.

Issue History

Date Modified Username Field Change
2014-02-20 23:49 Stratis Aravias New Issue
2014-02-20 23:57 Stratis Aravias Tag Attached: memory leak
2014-02-20 23:57 Stratis Aravias Tag Attached: TDBImage
2014-02-21 13:12 Reinier Olislagers Note Added: 0073190
2014-02-21 13:13 Reinier Olislagers File Added: dbimage_memleak.diff
2014-02-21 14:21 Stratis Aravias Note Added: 0073192
2014-02-21 15:00 Reinier Olislagers Note Added: 0073195
2014-02-21 18:33 Dennis Fehr Note Added: 0073198
2014-02-21 18:35 Dennis Fehr Note Edited: 0073198 View Revisions
2014-02-21 21:39 Marco van de Voort Note Added: 0073201
2014-02-22 00:32 Stratis Aravias Note Added: 0073202
2014-02-22 00:43 Stratis Aravias Note Edited: 0073202 View Revisions
2014-02-25 10:23 Michael Van Canneyt Project FPC => Lazarus
2014-02-25 11:13 Reinier Olislagers Note Added: 0073281
2014-02-25 11:25 Reinier Olislagers Note Edited: 0073281 View Revisions
2014-03-03 20:09 Jesus Reyes Assigned To => Jesus Reyes
2014-03-03 20:09 Jesus Reyes Status new => assigned
2014-03-03 20:18 Jesus Reyes Fixed in Revision => 44334
2014-03-03 20:18 Jesus Reyes LazTarget => 1.4
2014-03-03 20:18 Jesus Reyes Status assigned => resolved
2014-03-03 20:18 Jesus Reyes Fixed in Version => 1.3 (SVN)
2014-03-03 20:18 Jesus Reyes Resolution open => fixed
2014-03-03 20:18 Jesus Reyes Target Version => 1.4