TTMSFMXTreeview Reorder Access Violation

FMX TMS FMX UI Pack
1

Hi,


In my program there's a treeview in which the end-user can reorder items.
Most of the time this goes without any problems, but now and then and Access Violation occurs.

From TTMSFMXCustomTreeView.HandleMouseUp the statement FDragNode.Node.MoveTo() is executed.

The problem seems to be in statement FTreeView.RemoveNode(Self);
Self gets freed in the RemoveNode method, so accessing fields of Self (like FTreeview) will/can cause problems.
Actually I find it strange that it goes well most of the time.

procedure TTMSFMXTreeViewNode.MoveTo(ADestination: TTMSFMXTreeViewNode; AIndex: Integer = -1);
var
  n, nw: TTMSFMXTreeViewNode;
begin
  if ADestination <> Self then
  begin
    if Assigned(FTreeView) then
      FTreeView.BeginUpdate;
    n := TTMSFMXTreeViewNode.Create(nil);
    n.Assign(Self);
    nw := ADestination.Nodes.Add;
    nw.Assign(n);
    n.Free;
    if Assigned(FTreeView) then
      FTreeView.RemoveNode(Self);
    if (AIndex >= 0) and (AIndex <= ADestination.Nodes.Count - 1) then
      nw.Index := AIndex;
    if Assigned(FTreeView) then
      FTreeView.EndUpdate;
  end;
end;

procedure TTMSFMXTreeViewData.RemoveNode(ANode: TTMSFMXTreeViewNode);
begin
  if Assigned(ANode) then
    ANode.Free;
end;
2

Hi, 


It's unclear why the access violation occurs, but assuming the FTreeView is being assigned a nil, the assignment check verifies the FTreeView is actually assigned before executing the EndUpdate. If the access violation occurs, there's most likely a different issue and this needs to be looked into. Can you prepare a sample or step by step instructions on how to reproduce this issue?
3

Hey Pieter,


If the error occurs and I'm debugging in Delphi, I can see that the contents of Self (in my watch window) is complete rubbish. That's logical because the object is cleared.
The problem is that we are still in a routine of the already destroyed object.
So even checking if FTreeView is assigned will create an error because that property is also gone.

The problem is that I myself can't even reproduce the error simply.
I have to try and try. But no "standard path" to achieve this.
All depends probably how/when Delphi or Windows decides to clean up or overwrite the memory. 
So creating a (simple) example program is not that easy.
I just have a Treeview with 8 items or so.
I then keep reordering them from sub levels to top level and back.
Only once in while the AV occurs
4

Hi, 


We'll investigate this here as soon as possible if we are able to reproduce this issue here, and if we can find a workaround for this issue.
5

Hopefully you find a fix/workaround soon because it's a real production issue at the moment.

6

BTW

We have a lengthy operation (few seconds) in the OnBeforeReorderNode.
If this process succeeds ACanReorder will be set to true.
7

Hi, 


We have tried to reproduce this issue here but unfortunately haven't been able to reproduce this so far. Could you provide a sample and step by step instructions on how to reproduce this issue?
8

Hi Pieter,


Like I mentioned in my 2nd text, I can't even reproduce it easy myself.
I probably depends on Windows (version) when memory is actually freed and the state your system is in (how many other applications are running, which percentage of memory is used or so).

But actually even without running the code, just looking at the code, it's obvious for me that problems can (and will) occur.
The code is running in a method of an object.
Within that method the object itself is freed (which is strange, I would think the Treeview should free it's nodes).
After freeing, there is still code in the (freed object)method to be executed.
And there's even a reference to a property of the freed object (FTreeview).

Not tested, but I think if we would change the RemoveNode to the following, the error would be more obvious.

procedure TTMSFMXTreeViewData.RemoveNode(var ANode: TTMSFMXTreeViewNode);
begin
  if Assigned(ANode) then
    FreeAndNil(ANode); //ANode.Free;
end;

9

Hi, 


We have moved the code to treeview level avoiding self.free to destroy the node. The next version will address this.

10

Great news Pieter.

Thx very much.

Is it a small change, which I can do over here as well ?
Or do I have to wait for the next release ? 

11

It actually involves moving the code to treeview level.

So basically create a method which has the following signature and code:

procedure TTMSFMXTreeViewData.MoveNode(ANode,
  ADestinationNode: TTMSFMXTreeViewNode; AIndex: Integer);
var
  n, nw: TTMSFMXTreeViewNode;
begin
  if ADestinationNode <> ANode then
  begin
    BeginUpdate;
    n := TTMSFMXTreeViewNode.Create(nil);
    n.Assign(Self);
    nw := ADestinationNode.Nodes.Add;
    nw.Assign(n);
    n.Free;
    RemoveNode(ANode);
    if (AIndex >= 0) and (AIndex <= ADestinationNode.Nodes.Count - 1) then
      nw.Index := AIndex;
    EndUpdate;
  end;
end;

and then call it inside the MoveTo method.

procedure TTMSFMXTreeViewNode.MoveTo(ADestination: TTMSFMXTreeViewNode; AIndex: Integer = -1);
begin
  if Assigned(FTreeView) then
    FTreeView.MoveNode(Self, ADestination, AIndex);
end;

This should avoid accessing private variables that might be un-initialized during destroy sequence of the node. If you apply this code, please provide feedback if this fixes the issue

12

Hi Pieter,


I made the changes to my TMS sources and the Access Violation doesn't seem to occur anymore.
So that's VERY good news.

However if the MoveNode method gets called, there is a display problem.
Most of the time MoveNode is NOT called, because the condition below results to False.

        if FDragNode.Level = dn.Level then
          FDragNode.Node.MoveTo(dn.Node.GetParent, dn.Node.Index);

One or more items in the treeview don't get repainted if the MoveTo is executed.
Maybe you can make a small test over there, forcing the MoveTo getting called.

13

Hi, 


Moving up a level causes Self to be the treeview instead of the node. We have now corrected this.
Please change

n.Assign(Self)

to 

n.Assign(ANode)
14

Hey Pieter,


That seems to do the trick.
I've made the change and I haven't been able to generate an Access Violation or Painting issue.
Feels much more stable now.

Thanks a lot for all your help.

15

Thank you for your feedback, the next version will have these fixes included.