1669 System instability

by **markstuartwalker** » Mon Dec 02, 2013 4:51 am

I have added the necessary memory releases etc. These are within 4.1.0.2960 which I posted today. The mis-behaving of the internal cache has gone.

I still get the exception upon exit in the Dev environment.

by **Ludek** » Wed Nov 27, 2013 10:33 am

markstuartwalker wrote: Q1: Do I need Critical sections in my code, I understood the plugins to be single threaded?

Yes, if your plugin is single threaded then you don't need a CS (I just copied the code from one of our native plugins)

markstuartwalker wrote: Q2: ZeroMemory() is not in standard libraries but I think that is of negligible benefit

ZeroMemory is part of Windows.pas unit that is part of Dephi 2010. It only fills the memory by zeros.

Code: Select all

procedure ZeroMemory(Destination: Pointer; Length: DWORD);
begin
  FillChar(Destination^, Length, 0);
end;

markstuartwalker wrote: Q3: Doesn't MM dispose of the memory once it has consumed the information? I do not retain a pointer the the results that I return from either of these functions.

No, the plugin is responsible to dispose the memory that it allocated. So if you doesn't dispose then there is a memory leak.

by **markstuartwalker** » Wed Nov 27, 2013 6:33 am

OK, I think that I can see that now. I have created a few functions to encapsulate this capability. Please review and confirm.

Q1: Do I need Critical sections in my code, I understood the plugins to be single threaded?
Q2: ZeroMemory() is not in standard libraries but I think that is of negligible benefit
Q3: Doesn't MM dispose of the memory once it has consumed the information? I do not retain a pointer the the results that I return from either of these functions.

Code: Select all

unit ScanResultUnit;

interface

uses sysutils,DeviceCommon;

function createScanResult(count:integer):PScanResult;
procedure addScanResult(res:PScanResult;
i:Integer;
                        name:string ;
                        filesize:integer ;
                        lastModified:TDateTime ;
                        track_cookie:integer);

implementation


function createScanResult(count:integer):PScanResult;
var
  size:integer;
begin
  new(Result);
  Result.FileCount := count;
  size := count * sizeof(Result.Files[0]);
  GetMem(Result.Files,size);

(*
  New( ScannedPlaylists);
    GetMem( ScannedPlaylists.Files, playlists.Count*sizeof(ScannedPlaylists.Files[0]));
*)

end;

procedure addScanResult(res:PScanResult;
                        i:Integer;
                        name:string ;
                        filesize:integer ;
                        lastModified:TDateTime ;
                        track_cookie:integer);
type
  PFileName = ^TFileName;
  TFileName = array [0 .. 255] of WideChar;
var
//  name2 : PfileName ;
  rec : PScanRecord;
begin
  if i < res.FileCount then
  begin

    new(res.Files[i]);
    rec:=res.files[i]; // for debug
//    ZeroMemory( res.Files[i], sizeof(res.Files[i]^));

    //new(name2);
    //StrCopy(@name2[1], PWideChar(name));
    GetMem( res.Files[i].filename, length(name)*2+2);
    StrCopy( res.Files[i].filename, PWideChar(name));

//    res.Files[i].filename := @name2[1];
    res.Files[i].filesize := filesize;
    res.Files[i].lastmodified := lastModified;
    res.Files[i].track_cookie := track_cookie;
  end;
end;

(*
function TiPodDB.GetScannedPlaylists: PScanResult;
var
  i : integer;
  s : String;
  plst : TiPodPlaylist;
begin
  EnterCriticalSection( iPodCS);
  try
    ClearScannedPlaylists;
    New( ScannedPlaylists);
    GetMem( ScannedPlaylists.Files, playlists.Count*sizeof(ScannedPlaylists.Files[0]));
    ScannedPlaylists.FileCount := 0;
    for i:=0 to playlists.Count-1 do
    begin
      plst := TiPodPlaylist( playlists[i]);
      if plst.MHYP.podcast_flag = 0 then
      begin
        New( ScannedPlaylists.Files[ScannedPlaylists.FileCount]);
        ZeroMemory( ScannedPlaylists.Files[ScannedPlaylists.FileCount], sizeof(ScannedPlaylists.Files[ScannedPlaylists.FileCount]^));
        s := plst.Title;
        GetMem( ScannedPlaylists.Files[ScannedPlaylists.FileCount].filename, length(s)*2+2);
        StrCopy( ScannedPlaylists.Files[ScannedPlaylists.FileCount].filename, PWideChar( s));
        inc( ScannedPlaylists.FileCount);
      end;
    end;
    result := ScannedPlaylists;
  finally
    LeaveCriticalSection( iPodCS);
  end;
end;

procedure TiPodDB.ClearScannedPlaylists;
var
  i : integer;
begin
  EnterCriticalSection( iPodCS);
  try
    if ScannedPlaylists<>nil then
    begin
      for i:=0 to ScannedPlaylists^.FileCount-1 do
      begin
        Dispose( ScannedPlaylists^.Files^[i].filename);
        Dispose( ScannedPlaylists^.Files^[i]);
      end;
      Dispose( ScannedPlaylists^.Files);
      Dispose( ScannedPlaylists);
      ScannedPlaylists := nil;
    end;
  finally
    LeaveCriticalSection( iPodCS);
  end;
end;*)

end.

by **Ludek** » Tue Nov 26, 2013 9:48 am

Mark yes, this is most probably the root of the problem.

You are not allocating memory for name2.

For example our iPod plugin uses this code:

Code: Select all

function TiPodDB.GetScannedPlaylists: PScanResult;
var
  i : integer;
  s : String;
  plst : TiPodPlaylist;
begin
  EnterCriticalSection( iPodCS);
  try
    ClearScannedPlaylists;
    New( ScannedPlaylists);
    GetMem( ScannedPlaylists.Files, playlists.Count*sizeof(ScannedPlaylists.Files[0]));
    ScannedPlaylists.FileCount := 0;
    for i:=0 to playlists.Count-1 do
    begin
      plst := TiPodPlaylist( playlists[i]);
      if plst.MHYP.podcast_flag = 0 then
      begin
        New( ScannedPlaylists.Files[ScannedPlaylists.FileCount]);
        ZeroMemory( ScannedPlaylists.Files[ScannedPlaylists.FileCount], sizeof(ScannedPlaylists.Files[ScannedPlaylists.FileCount]^));
        s := plst.Title;
        GetMem( ScannedPlaylists.Files[ScannedPlaylists.FileCount].filename, length(s)*2+2);
        StrCopy( ScannedPlaylists.Files[ScannedPlaylists.FileCount].filename, PWideChar( s));
        inc( ScannedPlaylists.FileCount);
      end;
    end;
    result := ScannedPlaylists;
  finally
    LeaveCriticalSection( iPodCS);
  end;
end;

procedure TiPodDB.ClearScannedPlaylists;
var
  i : integer;
begin
  EnterCriticalSection( iPodCS);
  try
    if ScannedPlaylists<>nil then
    begin
      for i:=0 to ScannedPlaylists^.FileCount-1 do
      begin
        Dispose( ScannedPlaylists^.Files^[i].filename);
        Dispose( ScannedPlaylists^.Files^[i]);
      end;
      Dispose( ScannedPlaylists^.Files);
      Dispose( ScannedPlaylists);
      ScannedPlaylists := nil;
    end;
  finally
    LeaveCriticalSection( iPodCS);
  end;
end;

where ScannedPlaylists : PScanResult is private member of class TiPodDB, and corresponds to Result struct in your code.
As you can see each filename gets its memory via GetMem() and is subsequently freed via Dispose() in TiPodDB.ClearScannedPlaylists.

So doing something similar should solve your troubles. Hope that helps.

by **markstuartwalker** » Sat Nov 23, 2013 12:48 pm

I think that I have pinned down that the root of the memory corruption. I have some cache space which is gutting overrun and it follows this memory usage.

The following works but I can't figure out if it is adequately allocating memory that it returns back to MM. Please give me some comments.

Code: Select all

function DeviceiTunesListPlaylists(DeviceHandle :Integer): PScanResult;
type
  PFileName = ^TFileName;
  TFileName = array [0 .. 255] of WideChar;
var
  lst: TStringList;
  i, size: integer;
  name: String;
  name2: PFileName;
begin
  // list of playlists please
  Lst := getListOfPlaylists(dev);

  // allocate the memory for the reply back
  new(Result);
  Result.FileCount := lst.count;
  size := lst.count * sizeof(PScanRecord);
  GetMem(Result^.Files,size);

  log.text(2, 'number and size', lst.count , size);

  // iterate through the list ...
  for i := 0 to lst.count - 1 do
  begin
    name := lst[i];
    log.text(2, i, 'playlist', name);
    new(name2);
    StrCopy(@name2[1], PWideChar(name));
    new(Result.Files[i]);
    Result.Files[i].filename := @name2[1];
    Result.Files[i].filesize := 0;
    Result.Files[i].lastmodified := 0;
    Result.Files[i].track_cookie := 0;

  end;

by **markstuartwalker** » Wed Nov 20, 2013 3:18 pm

The symptoms don't exhibit for normal users. It only shows when running within the Delphi development environment.

I have decided to publish to get some more feedback.

by **Ludek** » Tue Nov 19, 2013 2:47 pm

Mark, if I use your updated plugin http://www.mediamonkey.com/addons/brows ... or-itunes/ together with MMW build 4.1.0.1671 Debug I don't see any instability after syncing several playlists to iTunes. No problems on MM closing.

Are you sure that it is related to your addon exclusivelly? i.e. Does it happen only after syncing using your plugin? If yes, does the same symptomps happen with 4.0.7 too?

by **markstuartwalker** » Tue Nov 19, 2013 2:27 pm

Can anyone explain what GIT.GetInterfaceFromGlobal does?

Code: Select all

if (flags and dnfRemovePlaylist) <>0 then
      iTunesNotifyFile( DeviceHandle ,nil, FileName, flags)
    else
    begin
      GIT.GetInterfaceFromGlobal( TrackCookie, ISDBSongData, Track);
      iTunesNotifyFile( DeviceHandle ,Track, FileName, flags);
    end ;


function GIT : IGlobalInterfaceTable;

const
  DIID_ISDBUIButtonEvents: TGUID = '{CB4BFF25-1F10-495B-812A-AE78FDC0A163}';
  DIID_ISDBTreeNodeEvents: TGUID = '{4114837B-1B8C-43E9-9185-A3BC7BA40742}';
  DIID_ISDBApplicationEvents: TGUID = '{ABF1895B-1032-45E1-A119-8926831F7167}';

implementation

function GIT : IGlobalInterfaceTable;
const
  cGIT : IGlobalInterfaceTable = NIL;
//const
  CLSID_StdGlobalInterfaceTable : TGUID =
  '{00000323-0000-0000-C000-000000000046}';
begin
  if (cGIT = NIL) then
    CoCreateInstance (CLSID_StdGlobalInterfaceTable, NIL, CLSCTX_ALL,
      IGlobalInterfaceTable, cGIT);
  Result := cGIT;
end;

by **markstuartwalker** » Tue Nov 19, 2013 2:24 pm

Peke wrote:Ok One stupid thing I found pounding my head. Have you tried to update SongsDB_TLB.pas?

Oooooooooooo ...no! But doesn't seem to make any difference.

by **Peke** » Mon Nov 18, 2013 5:05 pm

Ok One stupid thing I found pounding my head. Have you tried to update SongsDB_TLB.pas?

by **markstuartwalker** » Mon Nov 18, 2013 3:49 pm

I have completely re-written the HashString function into proper classes to make it properly use count compliant. Still no change - I still get the exceptions on closure.

Code: Select all

unit HashString;

interface

uses Classes;

type
  THashStrStruct = class

  public
//  PPHashStrStruct = ^PHashStrStruct;
//  PHashStrStruct = ^THashStrStruct;
//  THashStrStruct = record
  public
    id : String;
    data : pointer;
    next : THashStrStruct;
    constructor Create(pid: String; pdata: pointer);
    destructor Destroy; override;
  end;

  TStrHasher = class
   private
    table : array of THashStrStruct;
    tablesize : cardinal;
    bitmask : cardinal;

  //  FRemoveObjects: boolean;
  //  FRemoveRecords: boolean;
    function HashValue( id:String) : cardinal;
   public
    constructor Create( log2size:integer);
    destructor Destroy; override;

    function Hash( id:String; data:pointer) : boolean;     // returns true if the id was already there
    function Find( id:String; out data:pointer) : boolean;
    function Delete( id:String) : boolean;

    function GetListOfAll : TList;
    function GetListOfAll2: TList;

//    property RemoveObjects : boolean read FRemoveObjects write FRemoveObjects;
//    property RemoveRecords : boolean read FRemoveRecords write FRemoveRecords;
  end;



end;

function TStrHasher.Delete(id: String): boolean;
var
  p,ptemp : THashStrStruct;
begin
  result := false;
//  if not CaseSensitive then
//    id := WideUpperCase( id);

  p := table[HashValue(id)];
  while p<>nil do
  begin
    if p.id=id then
    begin
      ptemp := p;
      p := ptemp.next;
      ptemp.Free;
      result := true;  // Found
      exit;
    end;
    p := p.next;
  end;
end;

destructor TStrHasher.Destroy;
var
  i : integer;
  p, ptemp : THashStrStruct;
begin
implementation

uses SysUtils;

constructor THashStrStruct.Create(pid: String; pdata: pointer);
begin
  id:=pid;
  data:=pdata;
end;

destructor THashStrStruct.Destroy;
begin
  id:='';
  data:=nil;
end;

{ TStrHasher }

constructor TStrHasher.Create(log2size: integer);
var
  i:integer;
begin
  tablesize := 1 shl log2size;
  bitmask := tablesize-1;
  SetLength( table, tablesize);
  //CaseSensitive := aCaseSensitive;
  for i:=0 to tablesize-1 do
    table[i]:=nil;  for i:=0 to tablesize-1 do
  begin
    p := table[i];
    table[i]:=nil;
    while p<>nil do
    begin
      ptemp := p;
      p := p.next;
      ptemp.free;
    end;
  end;
  inherited Destroy;
end;

function TStrHasher.Find(id: String; out data: pointer): boolean;
var
  p : THashStrStruct;
begin
//  if not CaseSensitive then
//    id := WideUpperCase( id);

  result := false;
  p := table[HashValue(id)];
  while p<>nil do
  begin
    if p.id=id then
    begin
      result := true;
      data := p.data;
      break;
    end;
    p := p.next;
  end;
end;

function TStrHasher.GetListOfAll: TList;
var
  i : integer;
  p : THashStrStruct;
begin
  result := TList.Create;
  for i:=0 to tablesize-1 do
  begin
    p := table[i];
    while p<>nil do
    begin
      result.Add( p.data);
      p := p.next;
    end;
  end;
end;

function TStrHasher.GetListOfAll2: TList;
var
  i : integer;
  p : THashStrStruct;
begin
  result := TList.Create;
  for i:=0 to tablesize-1 do
  begin
    p := table[i];
    while p<>nil do
    begin
      result.Add( p );
      p := p.next;
    end;
  end;
end;

function TStrHasher.Hash(id: String; data: pointer): boolean;
var
  p : THashStrStruct;
  i : integer;
begin
  i:=HashValue(id);
  if ( table[i] = nil ) then
  begin
     table[i]:=THashStrStruct.Create(id,data);
     result:=false;
     exit;
  end;

  p:=table[i].next;
  while p.next<>nil do
  begin
    if p.id=id then
    begin
      result := true;  // Found
      exit;
    end;
    p := p.next;
  end;

  // tage this on the end
  p.next:=THashStrStruct.Create(id,data);
  result := false;
end;

function TStrHasher.HashValue(id: String): cardinal;
var
  l : integer;
  pch : PWideChar;
begin
  result := 0;
  pch := PWideChar( id);
  for l:=length( id) downto 1 do
  begin
    result := byte( pch^) + (result shl 6) + (result shl 16) - result;
    inc( pch);
  end;
  result := result and bitmask;
end;

end.

by **markstuartwalker** » Mon Nov 18, 2013 9:55 am

I use the following with StrPCopy

Code: Select all

   if length( filename ) > 254 then
      filename:=copy(filename,1,254);
    StrPCopy(rec.filename, filename);

This is a bit moot as my filenames are simple numbers '\12345.mp3'. so this will never overrun.

Should I be assigning the "flags" parameter for anything? I don't currently and I've never seen it anything other than '0'.

by **Ludek** » Mon Nov 18, 2013 5:45 am

Hi, looking into our device plugins we use still:

Code: Select all

PFileNameRec = ^TFileNameRec;
  TFileNameRec = record
    filename: PWideChar;    // at least 256 chars wide buffer for filename
    track_id: integer;      // track id as in MM.DB file
    track_cookie: cardinal; // COM object SDBSongData can be accessed this way
    flags: integer;         // Not used at this moment
  end;

and

Code: Select all

  StrCopy( rec.filename, PWideChar( filename));

so actually also in our plugins the structure slightly differs and still uses PWideChar, so you should leave PWideChar there or PChar that is actually the same in D2010.

I guess that you uses this structure when modifying target paths (DEVICE_ModifyFilenames), right?
I see in our code that it reserves 256 chars long string, so maybe the instability could be caused if you would try to modify the path to a path longer than 256 chars, are you sure that you don't create longer paths?

by **markstuartwalker** » Mon Nov 18, 2013 4:12 am

Well, as expected, the WidetSring->String made no difference except a few compilation oddities.

Code: Select all

type
  PFileNameRec = ^TFileNameRec;
  TFileNameRec = record
    filename : String;    // at least 256 chars wide buffer
    track_id  : integer;
    track_cookie : cardinal;
    flags : integer;
  end;

...  
filename: String;  
rec: PFileNameRec;
  
// insert the filename
  StrCopy(PWideChar(rec.filename), PWideChar(filename));

Which did not result in the expected null terminated string, the content of the string was copied but not the termination. Any advice?

by **Ludek** » Fri Nov 15, 2013 6:18 am

Yes, you can try to change WideString -> String in your plugin to see if it work better, but my guess is that the problem needs to be elsewhere.
Maybe (if you can regularly reproduce the instability) you could try to call only some functions/interfaces to see which one causes the problem to be able easier find it.

1669 System instability

Post a reply

Expand view Topic review: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability

Re: 1669 System instability