EAccessViolation in PaintTree()

[joachimmarder]

We occasionally receive access violations tracked using MadExcept with this call stack:

exception class    : EAccessViolation
exception message  : Access violation at address 00E08562 in module 'Project.exe'. Read of address 00040010.

main thread ($98c):
00e08562 Project.exe System               263   +0 TObject.Free
011a5d7b Project.exe VirtualTrees       30631 +584 TBaseVirtualTree.PaintTree
00e098b6 Project.exe System               263   +0 @HandleFinally
77357657 ntdll.dll                                 RtlUnwind
7732c6b6 ntdll.dll                                 KiUserExceptionDispatcher
00eb0d91 Project.exe madExcept                     InterceptClassDestroy
00f9e739 Project.exe Vcl.Graphics                  TBitmapImage.Destroy
00f99f66 Project.exe Vcl.Graphics                  TCanvas.CreateFont
00f99f37 Project.exe Vcl.Graphics                  TCanvas.RequiredState
00f99e25 Project.exe Vcl.Graphics                  TCanvas.GetHandle
01159f07 Project.exe VirtualTrees.Utils   149   +2 SetCanvasOrigin
011a4b77 Project.exe VirtualTrees       30185 +138 TBaseVirtualTree.PaintTree
0119baec Project.exe VirtualTrees       23573  +23 TBaseVirtualTree.Paint
00fcf33b Project.exe Vcl.Controls                  TCustomControl.PaintWindow
00fc968d Project.exe Vcl.Controls                  TWinControl.PaintHandler
00fc9e78 Project.exe Vcl.Controls                  TWinControl.WMPaint
00fcf2d5 Project.exe Vcl.Controls                  TCustomControl.WMPaint
01191da0 Project.exe VirtualTrees       17446   +8 TBaseVirtualTree.WMPaint
00fc49f5 Project.exe Vcl.Controls                  TControl.WndProc
00fc94c2 Project.exe Vcl.Controls                  TWinControl.WndProc
0119e92e Project.exe VirtualTrees       25299  +32 TBaseVirtualTree.WndProc
00fc8afc Project.exe Vcl.Controls                  TWinControl.MainWndProc
00f47270 Project.exe System.Classes                StdWndProc
7732c68f ntdll.dll                                 KiUserCallbackDispatcher
76bc9854 ntdll.dll                                 DispatchMessageW
010d6feb Project.exe Vcl.Forms                     TApplication.ProcessMessage
010d702e Project.exe Vcl.Forms                     TApplication.HandleMessage
010d7361 Project.exe Vcl.Forms                     TApplication.Run
76e7919d KERNEL32.DLL                              BaseThreadInitThunk

main thread ($98c), inner exception level 1:
>> EAccessViolation, Access violation at address 00E08562 in module 'Project.exe'. Read of address 00040000
00e08562 Project.exe System               263   +0 TObject.Free
011a5d7b Project.exe VirtualTrees       30631 +584 TBaseVirtualTree.PaintTree
00e098b6 Project.exe System               263   +0 @HandleFinally
77357657 ntdll.dll                                 RtlUnwind
7732c6b6 ntdll.dll                                 KiUserExceptionDispatcher
00eb0d91 Project.exe madExcept                     InterceptClassDestroy
00f9e739 Project.exe Vcl.Graphics                  TBitmapImage.Destroy
00f99e25 Project.exe Vcl.Graphics                  TCanvas.GetHandle
01159f07 Project.exe VirtualTrees.Utils   149   +2 SetCanvasOrigin
011a4b77 Project.exe VirtualTrees       30185 +138 TBaseVirtualTree.PaintTree
0119baec Project.exe VirtualTrees       23573  +23 TBaseVirtualTree.Paint
00fcf33b Project.exe Vcl.Controls                  TCustomControl.PaintWindow
00fc968d Project.exe Vcl.Controls                  TWinControl.PaintHandler
00fc9e78 Project.exe Vcl.Controls                  TWinControl.WMPaint
00fcf2d5 Project.exe Vcl.Controls                  TCustomControl.WMPaint
01191da0 Project.exe VirtualTrees       17446   +8 TBaseVirtualTree.WMPaint
00fc49f5 Project.exe Vcl.Controls                  TControl.WndProc
00fc94c2 Project.exe Vcl.Controls                  TWinControl.WndProc
0119e92e Project.exe VirtualTrees       25299  +32 TBaseVirtualTree.WndProc
00fc8afc Project.exe Vcl.Controls                  TWinControl.MainWndProc
00f47270 Project.exe System.Classes                StdWndProc
7732c68f ntdll.dll                                 KiUserCallbackDispatcher
76bc9854 ntdll.dll                                 DispatchMessageW
010d6feb Project.exe Vcl.Forms                     TApplication.ProcessMessage
010d702e Project.exe Vcl.Forms                     TApplication.HandleMessage
010d7361 Project.exe Vcl.Forms                     TApplication.Run
76e7919d KERNEL32.DLL                               BaseThreadInitThunk

main thread ($98c), inner exception level 2:
>> EAccessViolation, Access violation at address 00F98022 in module 'Project.exe'. Read of address 00040010
00f98022 Project.exe Vcl.Graphics                  TFont.GetHandle
00f99f66 Project.exe Vcl.Graphics                  TCanvas.CreateFont
00f99f37 Project.exe Vcl.Graphics                  TCanvas.RequiredState
00f99e25 Project.exe Vcl.Graphics                  TCanvas.GetHandle
01159f07 Project.exe VirtualTrees.Utils   149   +2 SetCanvasOrigin
011a4b77 Project.exe VirtualTrees       30185 +138 TBaseVirtualTree.PaintTree
0119baec Project.exe VirtualTrees       23573  +23 TBaseVirtualTree.Paint
00fcf33b Project.exe Vcl.Controls                  TCustomControl.PaintWindow
00fc968d Project.exe Vcl.Controls                  TWinControl.PaintHandler
00fc9e78 Project.exe Vcl.Controls                  TWinControl.WMPaint
00fcf2d5 Project.exe Vcl.Controls                  TCustomControl.WMPaint
01191da0 Project.exe VirtualTrees       17446   +8 TBaseVirtualTree.WMPaint
00fc49f5 Project.exe Vcl.Controls                  TControl.WndProc
00fc94c2 Project.exe Vcl.Controls                  TWinControl.WndProc
0119e92e Project.exe VirtualTrees       25299  +32 TBaseVirtualTree.WndProc
00fc8afc Project.exe Vcl.Controls                  TWinControl.MainWndProc
00f47270 Project.exe System.Classes                StdWndProc
7732c68f ntdll.dll                                 KiUserCallbackDispatcher
76bc9854 ntdll.dll                                 DispatchMessageW
010d6feb Project.exe Vcl.Forms                     TApplication.ProcessMessage
010d702e Project.exe Vcl.Forms                     TApplication.HandleMessage
010d7361 Project.exe Vcl.Forms                     TApplication.Run
76e7919d KERNEL32.DLL                              BaseThreadInitThunk

I have no idea what causes or triggers the exception. It is also not possible to handle this case in the projects code in a proper way. Any ideas are welcome.

[joachimmarder]

Idea: Extract code that resizes NodeBitmap and that is included more than once in PaintTree() to new helper function ReinitBitmap(). In a second step, do not resize but re-create the bitmap if different dimensions are required. Advantages:

• The constructor of TBitmaphas a small overhead, compared to what might happen if you resize a bitmap (e.g. CopyBitmap())
• Starting with a fresh bitmap may avoid the AV above

[the-Arioch]

011a5d7b Project.exe VirtualTrees 30631 +584 TBaseVirtualTree.PaintTree 00e098b6 Project.exe System 263 +0 @HandleFinally

Weird... What should it mean? some finally-block is immediately and directly calling PaintTree?

[the-Arioch]

Can you ask those users about

• what is the value of poUnbuffered in PaintOptions ?
• what is the value of PaintTree(TargetCanvas ?

To me it seems like they try to render it onto some invisible canvas that does not have neither handle nor font for itself.

I stumbled upon similar issues, if i remember it was in JvUltimDBGrid around ReCreateWND when it wrapped around custom sorting and tried to re-paint the non-existing canvas

I especially cherish the following comment there below...

  // Lock the canvas to avoid that it gets freed on the way.
  PaintInfo.Canvas.Lock;

They also seem to be using quite old VTW: in vanilla trunk VTW line number "30185" stands for PaintWidth := Window.Right - Window.Left; with Window being TRect. Thus it can not issue the call to SetCanvasOrigin - and there are plenty of those!

Personally I perhaps would start with a guard: right before the PaintInfo.Canvas.Lock; I would add a guard

if nil = PaintInfo.Canvas then exit; // and do nothing if nowhere to draw pon if not PaintInfo.HandleAllocated then exit; // and do nothing if nowhere to draw upon

[the-Arioch]

"code that resizes NodeBitmap" actually clears TBitmapCanvas.FHandle, via TBitmap.SetSize -> CopyImage -> FreeContext, so the guard described above may be not enough...

[the-Arioch]

TCanvas.GetHandle is a rather complex function, I'd better cache Canvas.Handle inside SetCanvasOrigin into a local var

[the-Arioch]

There is a number of calls to SetCanvasOrigin(PaintInfo.Canvas, 0, 0); in PaintTree; I guess the SetCanvasOrigin better saw resetting as a special case and saved two GDI calls

procedure SetCanvasOrigin(Canvas: TCanvas; X, Y: Integer);

// Set the coordinate space origin of a given canvas.

var
  P: TPoint;
  HC: THandle;

begin
   HC := Canvas.Handle;

  // Reset origin as otherwise we would accumulate the origin shifts when calling LPtoDP.
  SetWindowOrgEx(HC, 0, 0, nil);

   if (X=0) and (Y=0) then exit;

  // The shifting is expected in physical points, so we have to transform them accordingly.
  P := Point(X, Y);
  LPtoDP(HC, P, 1);

  // Do the shift.
  SetWindowOrgEx(HC, P.X, P.Y, nil);
end;

[joachimmarder]

some finally-block is immediately and directly calling PaintTree?

I guess we need to look at the call stack of the inner exception.

[joachimmarder]

what is the value of poUnbuffered in PaintOptions

poUnbuffered was not used.

They also seem to be using quite old VTW

Yap, could be V5.5 or V6.0.

[joachimmarder]

better saw resetting as a special case and saved two GDI calls

Your code presumes that (0,0) in device pixels is always (0,0) in logical pixels. But cannot viewport or world transformations change that?

[the-Arioch]

at least handle caching is not related to coordinates :-)

[the-Arioch]

"SetWindowOrgEx specifies which logical point maps to the device point (0,0). It has the effect of shifting the axes so that the logical point (0,0) no longer refers to the upper-left corner."

So after the first unconditional SWOE call I really do presume "(0,0) in device pixels is always (0,0) in logical pixels" and I think so does MSDN :-)

[joachimmarder]

at least handle caching is not related to coordinates :-)

Oh, come on, the overhead in TCanvas.GetHandle() is minimal in case the handle is already there. I prefer the clean code and stick with Donald Knuth in such cases ("Premature optimization is the root of all evil").

[the-Arioch]

How is local variable not a clean code? You get the value once and then if you ever would need to change it so some other canvas - you would only need to change one line, instead of copy-pasting all the calls to Canvas.GetHandle with risk to overlook some. Introducing local vars is introduicing single responsibility entities thus is cleaner code.

Just imagine if we remove PaintInfo.Canvas in PaintTree and copy-paste the same if unbufferred then-else everywhere we need to work with one or another canvas ? Would it make code cleaner? That is the same. Obtain the value once, use it many times, if in future decide you need to obtain another value - change one single responsible line instead of searching through all the code.

additionally what if between those three calls to Canvas.GetHandle that handle would be changed from some another thread, for example with something like that very Height:=0 ?

PS. I wish Delphi had variables that can only be assigned (or "bound") once as many other languages do... That would really help with cleaner code :-D

[joachimmarder]

So after the first unconditional SWOE call I really do presume "(0,0) in device pixels is always (0,0) in logical pixels" and I think so does MSDN

This applies to SetWindowOrgEx(), but couldn't other transformations apply, like defined through SetWorldTransform() or SetViewportExtEx()?

[the-Arioch]

When can they be applied ? in between calls to SWOE and LPtoDP from another thread?

Other transformations may scale and turn radius-vectors, but when the radius-vector is zero (a dot) - then you can scale and turn it however you want - it still would be a dot. SWOE maps (0,0) to (0,0). After that the (0,0) IS (0,0). Indeed, other dots then define non-zero radius-vectors that can be transformed to point to another dot after it. But the very center dot (0,0) that being mapped - defines zero-vector that you can multiply to any transformation matrix - zero multiplied still remains zero.

By definition of SWOE it ensures (0,0) == (0,0) with all and any current active transformations. That is exactly its essence as described in MSDN.

[the-Arioch]

If anything that very comment

// Reset origin as otherwise we would accumulate the origin shifts when calling LPtoDP.

proves my idea about mapping the selected points

it makes sense exactly because the prior calls ensures with any current transformation " (0,0) IS (0,0). "

I don't see any other possible meaning in that comment

[joachimmarder]

When can they be applied ? in between calls to SWOE and LPtoDP from another thread?

That's not what I meant. Does SWOE(0,0) reset all transformation including world transformations? I am not sure, and this is why I don't want to change the code which is working fine as it is now.

I came to the VTV project because all its bugs drove me mad. It is relatively stable meanwhile, at least those features that I use in my daily work, and I want to keep it that way. Which brings me back to D. Knuth.

Other transformations may scale and turn radius-vectors,

AFAIK SetWorldTransform() may also shift the coordinate system.

[the-Arioch]

SetWorldTransform() may also shift the coordinate system.

yes it can, if called IN BETWEEN SWOE and LPtoDP - but it is not being called there, is it?

Does SWOE(0,0) reset all transformation including world transformations?

No, it does not. It introduces extra transformation (shift) that enforces (0,0) == (0,0). All other points mapping would be changed accordingly. Unless some new post-SWOE transformation would shift it yet again - that mapping between those two specific points would remain.

Think about is as two planes made of latex. you stretch them both how your heart wants, then to select a point in both and put a needle throw those points, mapping them into one union point. Now you may turn and stretch the planes around that common, anchored point. And other points would be changing their mapping, but not the points fixed by the needle.

[joachimmarder]

How is local variable not a clean code?

Well, someone (not me) wrote this code that way, someone else (again not me) moved it to the new "VirtualTrees.Utils" unit and also decided not to change it. This is always a matter of personal preference, but I prefer not to have variables of platform specific types and I prefer not to change code that is running fine and has not proven to be a performance bottleneck. But I giess we could discuss this endlessly....

[the-Arioch]

not to have variables of platform specific types

we are talking about VCL here, more so we are talking about VTV that is intimately bound to WinGDI platform. There were many promises to make VTV for DotNet - but it was never done.

VTV is platform specific - from the ear-tip to the tail-end

that is running fine

That change would make it more maintainable for the future.

But of course that is YOUR repository not mine.

[joachimmarder]

we are talking about VCL here

I don't change the way I code just because I am doing VCL or .Net or FMX at the moment.

That change would make it more maintainable for the future.

I see why it is slightly faster, but I don't see why it should be more maintainable. I case the property type changes, I need to change the local var type as well. And we had several such API type changes changes in the past of Delphi, especially when they started the Delphi.Net stuff and when they made the 64Bit compiler.

[the-Arioch]

why it should be more maintainable.

because like i said above you introduce single responsibility principle

like i said above there would be one var to denote the HDC you work with, the var that can only be changed by the procedure itself, and the single line that queries the value. So modifying behaviour later would not become search-and-replace with re-analysing flow control.

Just imagine if we remove (another local variable) PaintInfo.Canvas in PaintTree and copy-paste the same if unbufferred then-else everywhere we need to work with one or another canvas ? Would it make code cleaner?

PS. Also, because external thread would not be able to change Canvas.Handle while we are inside this procedure. But since VCL and VTV are mostly uni-thread libs that is of lesser concern.

[joachimmarder]

I have not seen this exception with the latest versions of Virtual TreeView, so I am closing this issue as it was related to an older version.