View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002887JEDI VCL00 JVCL Componentspublic2005-04-18 01:132005-06-11 02:43
ReporterschuettecarstenAssigned Toobones 
PrioritynormalSeverityminorReproducibilitysometimes
Status resolvedResolutionfixed 
Product Version 
Target VersionFixed in Version3.10 
Summary0002887: GPF in TJvMRUList
DescriptionAfter the changes from 0002758 I sometimes get a GPF in InternalGetItem. This error occures when I call "EnumItems" immediately after "AddString", but it looks like this does not happen always. I can reproduce it here with special values for AddString, but with other values it works fine.

Take a look at the additional information for more details.
Additional InformationHere is a callstack:

Stack list, generated 18.04.2005 09:46:42
[00402B1B] System.@FreeMem + $13
[00402C5A] System.ErrorAt + $16
[00402B1B] System.@FreeMem + $13
[004C6291] JvMRUList.TJvMruList.SetItemData (Line 525, "JvMRUList.pas" + 4) + $0
[004C6102] JvMRUList.TJvMruList.InternalGetItem (Line 406, "JvMRUList.pas" + 21) + $5
[004052F1] System.@LStrArrayClr + $21
[004C6062] JvMRUList.TJvMruList.GetItem (Line 377, "JvMRUList.pas" + 0) + $2
[004C604A] JvMRUList.TJvMruList.EnumItems (Line 347, "JvMRUList.pas" + 6) + $4

The source code lines which create this error are:

  with rcbSource do
  begin
    rcbSource.Items.Clear;
    JvMruListSource.AddString(FileNames[1]);
    JvMruListSource.EnumItems;
    rcbSource.Text := FileNames[1];
  end;

FileNames[1] is String, when it's value is 'v:\projects\jedivcs\src\jedivcside\delphi_wizard\jvcsidewizard.pas' I get the error. The value 'v:\projects\jedivcs\src\jedivcside\delphi_wizard\jvcsotatestwindow.pas' works fine.

The registry for this MRU list shows the following values (copied from Regedit export):

[HKEY_CURRENT_USER\Software\JEDI\JEDIVCSDiff\MRU\Base]
"a"="v:\\projects\\jedivcs\\src\\jedivcs\\assignlabels.pas"
"MRUList"="hgejacifdb"
"b"="d:\\entwicklung\\metropolis\\metro\\m_artikelsuchen.pas"
"c"="v:\\projects\\jedivcs\\src\\jedivcs\\create.pas"
"d"="d:\\entwicklung\\metropolis\\metro\\m_tdbd.pas"
"e"="v:\\projects\\jedivcs\\src\\common\\jvcsideinterfacecommon.pas"
"f"="d:\\entwicklung\\metropolis\\metro\\m_auftragsbedarf.pas"
"g"="v:\\projects\\jedivcs\\src\\jedivcside\\delphi_wizard\\jvcsotatestwindow.pas"
"h"="v:\\projects\\jedivcs\\src\\jedivcside\\delphi_wizard\\jvcsidewizard.pas"
"i"="d:\\entwicklung\\komponenten\\vgkdb\\source\\visidbcheckbox.pas"
"j"="v:\\projects\\jedivcs\\src\\jedivcs\\projadmin.pas"
TagsNo tags attached.

Relationships

child of 0002758 resolvedobones Memory leaks in MRUList 

Activities

obones

2005-04-18 05:45

administrator   ~0007021

Ok, try this:

In JvMRUList.pas, in InternalGetItem, every time you find a call to SetItemData(P), set P to nil right after it.

This will prevent the "double free" problem, but I think the memory leak will be back. It will be less dramatic, but still, there is a memory management problem here that needs to be resolved.
Could you at least tell me that the above mentionned change solves the AV?

schuettecarsten

2005-04-18 06:55

developer   ~0007022

i tried, but it did not help. here are some changes to tjvmrulist.pas that will fix the problem, but i am not sure if they are clever. the problem is that the memory is freed twice in the current implementation.

Here is the new TJvMruList.Close procedure:

---
procedure TJvMruList.Close;
var
  P: Pointer;
begin
  if FList <> 0 then
  begin
    FreeMruList(FList);
    FList := 0;
  end;

  FItemIndex := -1;
  
  P := nil;
  SetItemData(P);
  if Assigned(P) then
    FreeMem(P);
end;
---

Here is the new TJvMruList.SetItemData procedure, please also change the declaration in the interface section:

---
procedure TJvMruList.SetItemData(var P: Pointer);
var
  Old: Pointer;
begin
  if FItemData.P <> P then
  begin
    Old := FItemData.P;
    FItemData.P := P;
    P := Old;
  end;
end;
---

obones

2005-04-22 05:38

administrator   ~0007039

I definitely don't like those changes.
It would be better to simply do
P := FItemData.P before calling SetItemData

Besides, the FItemData record only stores the last created pointer, and as such the memory leaks are still there. I have to find an elegant solution to this, but haven't yet.

schuettecarsten

2005-04-22 08:16

developer   ~0007050

you are right, my changes are really worse, but i needed a working solution very fast. i will try your solution and gibe feedback soon.

schuettecarsten

2005-05-11 13:56

developer   ~0007129

the problem is still there...

obones

2005-05-14 13:16

administrator   ~0007156

Yes, I know, I'll look into it as soon as I can

obones

2005-05-15 09:50

administrator   ~0007169

Replace the code for TJvMruList.InternalGetItem with this one:

function TJvMruList.InternalGetItem(Index: Integer; FireEvent: Boolean): Boolean;
var
  I: Integer;
  P: Pointer;
  EnP: TEnumMruList;
  CanFree: Boolean;
begin
  Result := False;
  if FList = 0 then
    Exit;
  P := nil;
  CanFree := True;

  try
    if FType = dtString then
    begin
      if not UseUnicode then
      begin
        ReAllocMem(P, 256);
        I := EnumMruList(FList, Index, P, 256);
        if I > 255 then
        begin
          ReAllocMem(P, I + 1);
          I := EnumMruList(FList, Index, P, I + 1);
        end;
        if I <> -1 then
        begin
          Result := True;
          SetItemData(P);
          CanFree := False;
          FItemIndex := Index;
          if FireEvent then
            DoEnumText
        end;
      end
      else
      begin // Unicode
        ReAllocMem(P, 512);
        I := EnumMruListW(FList, Index, P, 256);
        if I > 255 then
        begin
          ReAllocMem(P, (I + 1) * 2);
          I := EnumMruListW(FList, Index, P, I + 1);
        end;
        if I <> -1 then
        begin
          Result := True;
          SetItemData(P);
          CanFree := False;
          FItemIndex := Index;
          if FireEvent then
            DoUnicodeEnumText;
        end;
      end
    end
    else // FType = dtBinary
    begin
      ReAllocMem(P, 1024);

      if UnicodeAvailable then
        EnP := EnumMruListW
      else
        EnP := EnumMruList;
      //Arioch: work-around MS bug

      I := EnP(FList, Index, P, 1024);

      if I >= 1024 then
      begin
        ReAllocMem(P, 64000); // Arioch: Hmmm We'll never guess how much may there appear :)
        I := EnP(FList, 0, P, 64000);
      end;

      if I <> -1 then
      begin
        Result := True;
        ReAllocMem(P, I);
        // Arioch: should we waste more memory than we need?
        // and we can know the size of memory allocated
        // with GetMem and ReAllocMem, so we know how big Data was
        SetItemData(P);
        CanFree := False;
        FItemIndex := Index;
        if FireEvent then
          DoEnumData(I);
      end;
    end;
  finally
    // Free the memory
    if Assigned(P) and CanFree then
      FreeMem(P);
  end;
end;


Please let me know how it goes.

obones

2005-06-11 02:43

administrator   ~0007420

The proposed changes are now in CVS. This should fix the issue.

Issue History

Date Modified Username Field Change
2005-04-18 01:13 schuettecarsten New Issue
2005-04-18 01:13 schuettecarsten Relationship added child of 0002758
2005-04-18 05:24 obones Status new => assigned
2005-04-18 05:24 obones Assigned To => obones
2005-04-18 05:45 obones Note Added: 0007021
2005-04-18 05:45 obones Status assigned => feedback
2005-04-18 06:55 schuettecarsten Note Added: 0007022
2005-04-22 05:38 obones Note Added: 0007039
2005-04-22 08:16 schuettecarsten Note Added: 0007050
2005-05-11 13:56 schuettecarsten Note Added: 0007129
2005-05-11 13:56 schuettecarsten Status feedback => assigned
2005-05-14 13:16 obones Note Added: 0007156
2005-05-15 09:50 obones Note Added: 0007169
2005-05-15 09:50 obones Status assigned => feedback
2005-06-11 02:43 obones Status feedback => resolved
2005-06-11 02:43 obones Resolution open => fixed
2005-06-11 02:43 obones Note Added: 0007420